Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add __traits(getCmdlineConstant) #15257

Closed
wants to merge 1 commit into from
Closed

Add __traits(getCmdlineConstant) #15257

wants to merge 1 commit into from

Conversation

ErnyTech
Copy link

@ErnyTech ErnyTech commented May 21, 2023

This will introduce a solution similar in concept to the preprocessor macros commonly used in C/C++, but adapted to D which has no preprocessor.

In particular, allowing you to define a compile time constants by passing them to the compiler as a parameter.
Since that D doesn't have a preprocessor, I think the most elegant way to introduce a "global" constants, avoiding creating unwanted conflicts with code, is to introduce a traits capable of obtaining the constants defined via parameters passed to the compiler.

Currently some D-projects, including DMD itself, see

filename = FileName.combine(import("SYSCONFDIR.imp"), inifile);
try to sort of have constants defined at compiler params by writing the value to a file and then using ImportExpression, however I think this method is very inconvenient and introducing this new solution is simpler and more elegant.

Example code:

module mymod;
pragma(msg, __traits(getCmdlineConstant, "test1"));
pragma(msg, __traits(getCmdlineConstant, "test2", false, "default"));

Compiling with the parameters -define:mymod.test1=dlang will return the following
messages dlang and default, while compiling with
-define:mymod.test1=dlang -define:mymod.test2=dlang2 will return dlang and dlang2

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ErnyTech! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15257"

@ErnyTech ErnyTech changed the title Add __traits(getDefinedConstant) Add __traits(getCmdlineConstant) May 21, 2023
@thewilsonator thewilsonator added Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Severity:New Language Feature labels May 22, 2023
@rikkimax
Copy link
Contributor

I've been thinking a bit about configuration files instead of just a command-line argument. But that can be handled with a build manager like dub pretty easily. For this reason, I don't like calling it a command line constant because it may be backed by something else.

There is also some concern about long command lines, which is where a configuration file instead would be a better option.

I suspect there is some design work left to make this more compelling, but otherwise a nice idea.

@ErnyTech
Copy link
Author

For this reason, I don't like calling it a command line constant because it may be backed by something else.

This is a good argument, I'm not sure what the best name for it was for now I could think of:

  • getDefinedConstant but this is ambiguous, the user might think that it is used to get the constants defined in the code
  • getCmdlineConstant which is tied to the cmdline
  • getCompilerConstant again ambiguous, they don't look like user-defined constants

Other name ideas are welcome :-)

There is also some concern about long command lines, which is where a configuration file instead would be a better option.

I don't see this as a big deal because I imagine uses of this feature are done in projects that use build systems eg dub, cmake, makefile so command length isn't a real issue

I suspect there is some design work left to make this more compelling, but otherwise a nice idea.

Yes this is only necessary to introduce the concept in D, then it would be useful to add a convenient interface to use in dub to define the configuration and probably also some helpers in the std to make the most common uses more comfortable.

I'm trying to do this by minimizing language changes (just a new traits) and compiler then allowing usages to expand outside the language spec and DMD

I've been thinking a bit about configuration files

If this PR is merged, they can be implemented in DUB. I think it's better than introducing the need for the compiler to parse configuration files that probably should be defined in the language specification while with my solution the only change to the language is the new traits and the parameter to pass to the compiler is actually a implementation-defined

In any case, this PR is currently a draft, any advice to improve it would be welcome ;-)

@rikkimax
Copy link
Contributor

There is also some concern about long command lines, which is where a configuration file instead would be a better option.

I don't see this as a big deal because I imagine uses of this feature are done in projects that use build systems eg dub, cmake, makefile so command length isn't a real issue

It is. We've had a number of issues over the years regarding it in dub.

The general solution is to use a file to put the arguments in and pass that to the compiler, but unfortunately, that hasn't been 100%.

@ErnyTech
Copy link
Author

It is. We've had a number of issues over the years regarding it in dub.

Oh I've never seen this, can you send me a reference so I can learn? Thank you for your feedback

@rikkimax
Copy link
Contributor

Here are 4 although they appear to all be fixed by the same PR.

dlang/dub#2630
dlang/dub#2538
dlang/dub#2143
dlang/dub#911

@ErnyTech
Copy link
Author

Here are 4 although they appear to all be fixed by the same PR.

dlang/dub#2630 dlang/dub#2538 dlang/dub#2143 dlang/dub#911

As far as I can tell this is caused by the MS Linker, so a long command with -define shouldn't cause any problems

@PetarKirov
Copy link
Member

I think this is very useful addition to the compiler -> language interface! This lack of this functionality has necessitated lots of awkward workarounds in our ecosystem, usually requiring external build tools, for something that the compiler can handle by itself in a more ergonomic way.

At work we have a couple of Nim projects which is how I learned about the their -d, --define:SYMBOL(:VAL) compiler option, see:

And especially this:

nim c -d:FooBar=42 foobar.nim
const FooBar {.intdefine.}: int = 5

If -d:FooBar=42 is passed to the compiler when it's invoked then the FooBar would be set to 42 while otherwise it will remain 42.

There a few things that we can do better:

  • Using the module system for namespacing
  • Supporting arbitrary expressions

For example, for Phobos I'd like to be able to do the following:

dmd '-define:std.datetime.timezone.defaultTZDatabaseDir="${tzdata}/share/zoneinfo"'

Which would translate into:

enum defaultTZDatabaseDir = "/nix/store/w2swil1dw4fxmvkggcik4l0jiywwp5vp-tzdata-2022g/share/zoneinfo";

Or more generally, the syntax of the switch should be:
-define:<module.path>=<assign-expr>, where <assign-expr> is as defined in the D language specification - anything that is valid on the right hand sign of a =.

@ErnyTech
Copy link
Author

  • Using the module system for namespacing

I'm currently working on this

Supporting arbitrary expressions

I'm a bit skeptical about this, it risks causing confusion. Perhaps it would be better to add an optional parameter to traits to request evaluation at compile time

@rikkimax
Copy link
Contributor

I'm a bit skeptical about this, it risks causing confusion. Perhaps it would be better to add an optional parameter to traits to request evaluation at compile time

Attempting to tokenize it first (to get a single token out of it like a float), would be ok. If it fails fall back to string. But yeah an expression is more dodgy.

@ErnyTech
Copy link
Author

I'm a bit skeptical about this, it risks causing confusion. Perhaps it would be better to add an optional parameter to traits to request evaluation at compile time

Attempting to tokenize it first (to get a single token out of it like a float), would be ok. If it fails fall back to string. But yeah an expression is more dodgy.

The problem is a user could pass a string which could be evaluated as an integer, for example -define=foo=2 is this a string or integer?

Obviously we could force the user to specify the string like this -define=foo="2", but this leads to two problems: the presence of quite verbose quotas and the risk of bugs as it is very likely that a user forgets the quotas

@rikkimax
Copy link
Contributor

Yeah this could be configurable using a bool argument to the trait. No reason to not offer that override.

@PetarKirov
Copy link
Member

PetarKirov commented May 22, 2023

Regarding arbitrary expressions, here's one use case:

dmd '-define:org.project.app_version.currentVersion=SemVer(1, 2, 3)'

Obviously, this can be accomplished even if the compiler supported only passing strings, e.g.:

module app_version;

enum currentVersion = parseSemVer(__traits(getCmdlineConstant, "version"));

But the ability to pass expressions feels more ergonomic to me.

@ErnyTech
Copy link
Author

ErnyTech commented May 24, 2023

PR Updates:

  • Restricted the cmdline constants to the module, the user will have to specify the constant with the prefix of the fully qualified module name
# dmd -define:mymod.test1=some -define:test2=some

module mymod;
pragma(msg, __traits(getCmdlineConstant, "mymod.test1")); // Valid
pragma(msg, __traits(getCmdlineConstant, "test1")); // Implicit search for mymod.test1 --> Valid
pragma(msg, __traits(getCmdlineConstant, "test2")); // Constant not found!
  • Now the traits accepts in the second argument the type of the expression it expects to receive and will evaluate it at compile time
    If the second argument is not present or is "false" the evaluation will not be performed and a string exp of the unevaluated content of the constant will be obtained
    The third (optional) argument will now contain the default value and can contain any expression.
    In any case it will be checked if the type matches the one specified by the second argument
# dmd -define:mymod.test1=4

module mymod;
pragma(msg, __traits(getCmdlineConstant, "mymod.test1")); // String "4"
pragma(msg, __traits(getCmdlineConstant, "test1", false)); // String "4"
pragma(msg, __traits(getCmdlineConstant, "test1", int)); // Integer 4
pragma(msg, __traits(getCmdlineConstant, "__nodef", int, 2)); // Constant not found --> fallback default -> Integer 2
  • Now the cmdline option to specify constants is -define:<name>=<value> to remove the double "=" and make it less confusing

Thanks to @rikkimax and @PetarKirov who helped me with feedback to try and improve PR

@WebFreak001
Copy link
Member

Here are 4 although they appear to all be fixed by the same PR.

dlang/dub#2630
dlang/dub#2538
dlang/dub#2143
dlang/dub#911

all of those are problems with windows' file path length limit, nothing to do with command line length limits.

This will introduce a solution similar in concept to the preprocessor macros
commonly used in C/C++, but adapted to D which has no preprocessor.

In particular, allowing you to define a compile time constants by passing them
to the compiler as a parameter.
Since that D doesn't have a preprocessor, I think the most elegant way to
introduce a "global" constants, avoiding creating unwanted conflicts with code,
is to introduce a traits capable of obtaining the constants defined via parameters
passed to the compiler.

Currently some D-projects (including DMD itself, see https://github.com/dlang/dmd/blob/03ee26acb162dc92abf3ea1b9fc8e6296bb3eaa9/compiler/src/dmd/dinifile.d#L103)
try to sort of have constants defined at compiler params by writing the value to a
file and then using ImportExpression, however I think this method is
very inconvenient and introducing this new solution is simpler and more elegant.

Example code:
module mymod;
pragma(msg, __traits(getCmdlineConstant, "test1"));
pragma(msg, __traits(getCmdlineConstant, "test2", false, "default"));

Compiling with the parameters "-define:mymod.test1=dlang" will return the following
messages "dlang" and "default", while compiling with
"-define:mymod.test1=dlang -define:mymod.test2=dlang2" will return "dlang" and "dlang2"
ErnyTech pushed a commit to ErnyTech/dlang.org that referenced this pull request May 25, 2023
ErnyTech pushed a commit to ErnyTech/dlang.org that referenced this pull request May 25, 2023
@RazvanN7
Copy link
Contributor

cc @WalterBright

@ErnyTech ErnyTech marked this pull request as ready for review May 26, 2023 15:00
@ErnyTech ErnyTech requested a review from ibuclaw as a code owner May 26, 2023 15:00
@WalterBright
Copy link
Member

Thank you for taking the time to do this. I am aware this is a common feature of other languages, and deliberately did not include it with D. The rationale is the same as why D does not allow version expressions, e.g. things like version(A || B && C) { ... }. The idea is a project's binaries should be a closed set over its compilation parameters, not an open set. It's better when the implementer decides specifically what versions of the binaries are to be built, as then someone reading the source code will know the set of versions that it is designed to build. This also easily enables code to be built to prevent unanticipated binaries being built. C, as you noted, does have this feature, and it is not helpful in understanding C code that one didn't write. With the C preprocessor, I am constantly wondering what macros are actually defined and what their values are, especially when the build system for it is, well, where the heck is it? D tries to learn from C and avoided adding problematic features.

There are, however, workarounds as you have noted. These deliberately make things more difficult, as they should not be the first tool one turns to. One example used by the D compiler itself is the VERSION file, which contains the line:

v2.104.0

and then is used with the line:

private enum string _version = import("VERSION");

Personally, I would have preferred a file named config.d, with the contents:

enum string_version = "v2.104.0";

and then it could be simply imported with import config; but the issue there was other tools that also needed the version string, but without being wrapped in a D declaration.

Nevertheless, this works and serves the compiler's need.

To sum up, the lack of this feature being on the command line is deliberate, and intended to make the number of binaries that can be built a closed set. I appreciate the hard work you put into making this PR, but am sadly compelled to say no to it.

@ErnyTech
Copy link
Author

C, as you noted, does have this feature, and it is not helpful in understanding C code that one didn't write

It's true, #define often causes confusion in C

With the C preprocessor, I am constantly wondering what macros are actually defined and what their values are, especially when the build system for it is, well, where the heck is it? D tries to learn from C and avoided adding problematic features.

In fact my implementation doesn't introduce the problems of C #defines which mainly were:

  • #define is global, whereas this implementation restricts constants to modules
  • Unwanted conflict with code, this PR instead limits the constant to traits

My implementation actually currently looks more like Nim than C

Personally, I would have preferred a file named config.d, with the contents

Would a PR that allows auto-generating config.d be accepted? The ability to define configurations/constants without external tools is very useful, config.d is fine but by itself it doesn't cover the main use case i.e. allowing build system to define constants

There are, however, workarounds as you have noted. These deliberately make things more difficult, as they should not be the first tool one turns to

In fact, if users need this feature, it's better to provide them with a well-made solution than a workaround like import(file).

I literally have my own project in production full of import(files), it's disgusting to look at and difficult to manage. I'm considering whether to auto-generate a config.d somehow or keep an internal fork of D with this PR...

but am sadly compelled to say no to it.

Fine, but what is the alternative? a config.d alone is not enough and simply ignoring the need I think is also wrong

In any case, thanks for the review, I enjoyed doing this PR and I hope I can do something else that will eventually be accepted :-)

@rikkimax
Copy link
Contributor

Short of supporting this in the language with a trait, a configuration file using a fixed name that any build manager/compiler wrapper may employ with enum's (so it doesn't have anything to compile and can be freely overridden) would be the best solution.

So __configargs.d would be better than just config.d as that wouldn't be a module people should be writing which wouldn't be a breaking change.

rdmd and dub would be candidates for emitting this file. Dub should do that per-binary (not per package) and allow dependers to override vars that dependencies set. It should also be able to execute programs to get the value to set.

I was going to eventually refactor dub_test_root.d out to be available upon request, however implementing __configargs.d would do a large portion of that work.

@ErnyTech ErnyTech closed this by deleting the head repository Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review:Needs Rebase Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Walter Bright Severity:New Language Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants