add std.compiler.Version #8750
add std.compiler.Version #8750
Conversation
|
Thanks for your pull request and interest in making D better, @Herringway! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8750" |
As far as I know, the 'official' position is still that version logic is restricted by design. The standard library shouldn't endorse what the language is trying to avoid, that sends a mixed message. In any case, this needs to be approved by @atilaneves Personally I'm against this addition. The example in the changelog: static if (Version.D_InlineAsm_X86 || Version.D_InlineAsm_X86_64)
{
version = UseX86Assembly;
}Can easily be expressed by: version (D_InlineAsm_X86)
version = UseX86Assembly;
version (D_InlineAsm_X86_64)
version = UseX86Assembly;Also, mixing static if and version doesn't work properly, and the changelog example should arguably be deprecated: Issue 7386 |
The linked issue has not been marked WONTFIX. It seems to me that this is very much unresolved.
DRY. Especially here, where a compiler will be unable to help you with a typo. While it is true that this PR doesn't solve that entirely, it can cut down on the number of error-prone version specifications and that is an improvement, IMO.
I would personally argue for version specifications/conditions to be deprecated and removed entirely, as they require a valuable keyword (version), are redundant (static if) and dangerously error-prone (initial drafts of this PR had the wrong capitalization for D_InlineAsm_X86. very annoying). But I suspect that this would not be a popular move and pushing for it would not be a valuable use of my time. |
I can change that for you 🙂 although controversial issues tend to be reopened when that happens.
Has to do with redundant pieces of knowledge spread out over the project. It does not mean "factor out every common piece of source code you can", that results in an incomprehensible code golf submission. Otherwise I'd argue for support of static assert(Version.all);
static assert(!Version.none);Should become this: static assert(Version.all && !Version.none);Because 'don't repeat |
|
I aggree with Herringway, this should be supported by the compiler Example i had to do in my code: version(Windows)
alias socket_t = size_t;
else
alias socket_t = int;4 lines with repetition that could be just one liner, you never know you made a typo or missed something until you compile for that target, this is just bad UX, wastes time alias socket_t = version (Windows) size_t else int;or alias socket_t = version (Windows) ? size_t : int;regex in version however is imo a bad idea, slow and cryptic version should work as an expression, throw was made an expression recently, so what's preventing it?
restricting something doesn't mean making it impractical to use, actually it could mean plenty of things, but i'll always advocate for enabling nice and concise code, even if that mean going against the "official" stance |
|
I'll bring this up in the next DLF monthly meeting, but without new compelling arguments, I don't expect a different outcome. If you can demonstrate problems (other than it not looking nice) in existing projects that the current logic causes, you'll have a stronger case. |
Consider this code: version(linux) {
version = ExtraFunctionality;
} else version(OSX) {
version = ExtraFunctionaIity;
}
void foo() {
version(ExtraFunctionality) {
// fancy stuff here
}
// normal functionality
}Did you spot the error? The compiler sure didn't! Let's try a different example. version(linux) {
enum ExtraFunctionality = true;
} else version(OSX) {
enum ExtraFunctionaIity = true;
} else {
enum ExtraFunctionality = false;
}
void foo() {
static if (ExtraFunctionality) {
// fancy stuff here
}
// normal functionality
}Now we get a compiler error. Good. But only on one configuration, and it may not be the primary one used by the developers, so it can go unnoticed. enum ExtraFunctionaIity = Version.linux || Version.OSX;
void foo() {
static if (ExtraFunctionality) {
// fancy stuff here
}
// normal functionality
}Now the error is obvious on ALL systems, with the bonus of being much more concise and expressive. But there are still problems here. Adding more enums will naturally mean more Versions, which means more opportunities for undetectable typos. But we can fix this! enum LinuxVersion = Version.linux;
enum OSXVersion = Version.OSX;
enum ExtraFunctionality = LinuxVersion || OSXVersion;
enum CoolFunctionaIity = LinuxVersion;
enum NiftyFunctionality = OSXVersion;
void foo() {
static if (ExtraFunctionality) {
// extra stuff here
}
static if (CoolFunctionality) {
// cool stuff here
}
static if (NiftyFunctionality) {
// nifty stuff here
}
// normal functionality
}Is this ideal? No. But without language support giving us a tristate (ie undefined, true or false) version identifier in the future, I don't think we can do much better at this time. In conclusion, version() is dangerous and redundant. We can accomplish what version()'s goal was far more effectively with static if and enum, and we can do much better by moving beyond it. This PR merely implements a common partial solution that the community has already been using. |
|
That's a made up scenario.
Should probably be
Can you give an example of a dub project effectively using this? |
Okay. Version typos do happen, and can go undetected for a pretty long time.
Perhaps, but there is precedence in Phobos here. Lines 598 to 602 in cf97c75
No. Github does not make such a search feasible. But I can provide multiple forum posts and issues where this code has been suggested.
|
That's a good example of DRY: the list of reserved version identifiers is duplicated between the compiler and spec, making it easy for them to be out of sync. It's not something that would be prevented by version algebra though, that only prevents inconsistent spelling of an identifier within 10 lines of code.
That's a good example. Consider the
That's a nice list! But just because people present it as a proof of concept, doesn't mean it has been used successfully and battle tested in production. I think that when push comes to shove, users still rather use It's currently the norm that Phobos additions preferably first prove themselves successful as a dub package. For example, std.sumtype was developed and gained popularity on dub before its Phobos inclusion. Now, this PR's feature is a bit trivial for a dub package, but perhaps you can ask in the forum if anyone has successfully used a template like it. Because here's the thing: Walter has decades of experience with |
|
I don't think this merits inclusion in Phobos, especially as there's no reason anyone can't use this themselves right now. As @dkorpel said, I don't think the standard library should enable something that the language explicitly makes difficult. I understand that not everyone might agree on that stance, but the beauty is that solutions such as this one are possible anyway. |
So, time to to change This behavior of version creates a lot of bloat in the users modules (and even on DMD/Phobos/DRuntime codebase) and this alternative solves that bloat, without doing a syntax breaking change. The argument of "there's no reason anyone can't use this themselves right now" can be applied to the whole Phobos. And it ranges the simplest things as a Do we really need Windows specific registry utility in a standard library? That's dubious and it drills down, in the end, to what people need and their general concerns. |
Can you give examples of this? |
That's correct. |
The robust way to do this is: Then, when you inevitably add another OS, the compiler will point out where updates are needed. As for misspelling |
|
This PR is not approved. |
|
I don't have a position on this particular change, but I do have a philosophical question. What is the fundamental difference between For that matter, what is the fundamental difference between Why does the argument that complex boolean expressions make code hard to read not apply to It doesn't seem to me that a fundamental difference exists which would provide solid defense for the status quo. Perhaps we should either allow |
|
Thanks for the clarifying examples. @ljmf00
Note that Walter deliberately structured druntime to have simple duplication instead of complex factored out code:
druntime has a lot of version logic because it needs to inferface with several C runtimes on different platforms. It's not representative of user code.
It's imported once, but never used in that repository.
It's used once: static assert(isVersion!"PowerNex", ...);Which could also be expressed as version(PowerNex) {}
else static assert(0, ...);
That's a nice example.
Yes, ctod unfortunately has the ability to translate C's It inherited it from ctod, but it does provide usage examples. So I collected these examples: std.math.algebraicCurrently: version (linux) version = GenericPosixVersion;
else version (FreeBSD) version = GenericPosixVersion;
else version (OpenBSD) version = GenericPosixVersion;
else version (Solaris) version = GenericPosixVersion;
else version (DragonFlyBSD) version = GenericPosixVersion;This is 'bloat' and this is the proposed replacement: static if (Version.linux || Version.FreeBSD || Version.OpenBSD || Version.Solaris || Version.DragonFlyBSD)
version = GenericPosixVersion;Of perhaps using the Inline version logic in aeGitObject.TreeEntry(
isVersion!`Posix` && (de.attributes & octal!111) ? octal!100755 : octal!100644,
de.baseName,
writer.write(GitObject(Hash.init, "blob", cast(immutable(ubyte)[])read(de.name)))
)Translated C ifdefs in raylib-d:static if (!HasVersion!"RAYGUI_NO_ICONS" && !HasVersion!"RAYGUI_CUSTOM_ICONS")Or with this PR's syntax: static if (!Version.rayguiNoIcons && Version.rayguiCustomIcons)Are these good picks? I can show the examples to Walter, but I don't think they are very convincing, the tired old answer "it's not supposed to look nice and concise" still applies. |
It does apply. "no code" > "straightforward code" > "code with many branches". However, complex conditions can be required because your code solves a complex problem. It's best to have good test coverage where all paths are taken in that case. Version logic is different because it's almost always accidental complexity, and it's hard to test. When you have complex version logic, that sucks, but writing it as a concise one-liner is pure window dressing. |
|
One thing that I've noticed is that several of the examples used in the version() specification are much more concisely written with static if. version (ProfessionalEdition)
{
version = FeatureA;
version = FeatureB;
version = FeatureC;
}
version (HomeEdition)
{
version = FeatureA;
}
...
version (FeatureB)
{
// implement Feature B ...
}Could be: enum FeatureA = Version.ProfessionalEdition || Version.HomeEdition;
enum FeatureB = Version.ProfessionalEdition;
enum FeatureC = Version.ProfessionalEdition;
...
static if (FeatureB)
{
// implement Feature B ...
}Exhibit B: class Foo
{
int a, b;
version(full)
{
int extrafunctionality()
{
//...
return 1; // extra functionality is supported
}
}
else // demo
{
int extrafunctionality()
{
return 0; // extra functionality is not supported
}
}
}Could be: class Foo
{
int a, b;
int extrafunctionality()
{
static if (Version.full)
{
// ...
}
return Version.full;
}
}Exhibit C: version(linux) {
enum ExtraFunctionality = true;
} else version(OSX) {
enum ExtraFunctionality = true;
} else {
static assert(0, "system not accounted for");
}Could be: static if(Version.linux || Version.OSX) {
enum ExtraFunctionality = true;
} else {
static assert(0, "system not accounted for");
}What do we gain by having version()? Even if you prefer the original examples, they are still reproducible with static if in place of version(). I can tell you what we lose out on by having version(): a valuable identifier. It's a keyword, so we cannot have variables named 'version'. Do you know how often I see version fields in serialized structures? version() also has a consistency problem. Why are some predefined versions lowercase while the majority are uppercase (linux? Windows?)? Why are the underscores only sometimes used as word separators? The D compiler can't help us when we misspell these identifiers at all. There is no But that can't be done, because instead of the tri-state undefined/false/true you can achieve with typical booleans, we only have undefined/true. These are the things we were supposed to be leaving behind. The compiler can't help you if you misspell a version definition on the command line because of this. Why are users forced to "avoid #ifdef hell" instead of discovering and choosing that path on their own? I know I wouldn't willingly choose it from looking at the examples provided. If there's a benefit to this, it should be demonstrated, not simply asserted. Show, don't tell. Not only am I unconvinced by the status quo here, I find myself actively resenting it due to the feature's shortcomings mentioned above. Personally, I would rather see all predefined versions being made into simple namespaced boolean enums usable with a similar syntax as this PR. Version.nonexistent would be an error unless defined on the command line, Version.Windows would be true/false as appropriate. The only reasonable course of action is clear. version() must be eradicated in its entirety. It is dangerous and redundant. It will not be missed. |
That is kind of a no-argument to me. It's very workaround-ish to me and the reason for people to use it is maybe because they need it. Plus, this is the level of workarounds I need to deal with: version (A) version = A_;
else version (B) {}
else version (C) {}
else version = A_;
version (A_) version = AorB;
version (B) version = AorB;And no, this doesn't work: version (A) {}
else version (B) {}
else version (C) {}
else version = A;
version (A) version = AorB;
version (B) version = AorB; |
Is that code private? If you add back the context, and show how it would be improved by std.compiler.Version, it could be a stronger example in support of this PR. |
It is private. I can try to rewrite it with But, briefly, this is to be able to define a "default" version flag on the module, if no version flags are passed to the compiler. |
Is the argument for this PR now based on paving the way to replacing the version system entirely? That's a big breaking change, and would require even stronger rationale. |
Heh, that's funny. I recently did something very similar: dlang/dmd#15253 It would be nice to have a way to define a version flag as 'default' indeed, but that wouldn't require logical operators to be supported on version flags. |
No, I'm just thinking long-term. If adopted, a PR like this would make that less of a breaking change, though. |
It does apply: https://forum.dlang.org/post/u4f6vo$2b5q$1@digitalmars.com Complex conditionals are a rich source of bugs. |
They were generally selected to match the way that C compilers commonly cased their predefined macros. I received some flak years ago for naming the Linux version |
|
Since this PR discussion and my newsgroup post failed to find compelling examples to change the leadership's minds (#8750 (comment)), I don't think this is PR has a future. We'll have to agree to disagree here unfortunately. |
No proper justification for why it is not accepted, again. Another discussion to be added to the endless number of leadership's decisions with a biased opinion rather than logical pros and cons. This, in particular, is advocating for bloated source code with an horrible way to make branching with I believe we are slowly killing Phobos by deferring such essential helpers to third-party libraries. |
That's a very unfair summary
This PR should come with good justification why it should be accepted. With my inquiries I tried to help gather examples of real bugs caused by D's current version design that this PR prevents, or snippets of "problematic code before this PR vs. how it's solved after this PR", but didn't get any. All I got was:
|
|
Most of the examples where referenced from official code, not just a random library that uses this that way, and for me, seeing this the official way to do it, is concerning to me.
Maybe I see the definition of problematic more broadly, because problematic code is not just code that produces buggy results. Bloated code means more code to read. There are approaches to this that can still be maintainable and very readable, with less code. You might not see a problem on a language that would enforce you to not use logical operators in replacement to this idea, but I would. Imagine Instead of having: if ((A || B) && C)
return 4;
return 5;if (A)
if (C)
return 4;
if (B)
if (C)
return 4;
return 5;I'm sure there would be more bugs if such a language was extensively used by a a large codebase. |
Then where are the bugs caused by D being such a language with regards to version conditions? |
There's many reasons for why you shouldn't do code duplication, one of them is human errors. Code duplication is universally considered a code smell. See https://en.wikipedia.org/wiki/Duplicate_code . There's even academic studies on this https://elib.uni-stuttgart.de/handle/11682/3640 . e.g. getting a char wrong among a list of a few version defines can lead to a silent different behavior, for critical codebases can even lead to security vulnerabilities. That's not my biggest issue, although. People, as referenced by the links I sent, use kind of the same template to check This is a bit off-topic, but related to the negative feedback we often get: The same way there's other code smells that are devalued by D leadership. There are some very common code smells, like having unused variables, that I and my coworkers have already stumbled upon. And currently we have no way to reliably check them other than have a linter check after the semantic analysis in the compiler. Any decent compiler has built-in linter checks, so people don't need to re-run semantic checks on each build. And this is a concern on industry codebase, where build times get really important. Last time I had this conversation somewhere, @WalterBright denied the proposal of adding such rules to DMD. |
This isn't code duplication. #8750 (comment)
The paper you linked says the opposite. "there are contradictory results regarding the connection of cloning and faults"
It leads to a compilation error, #8750 (comment) (second paragraph) |
Noted. Do you want to discuss this somewhere else? |
I am hesitant to call this a fix for issue 7417, because I think there is still some merit to the idea as proposed, but this is a common-enough workaround that I feel it is an appropriate for inclusion in Phobos. The rationale both for and against this has been discussed to death already, and can be found in the issue as well as the many forum threads that have come up over the years.
I am unsure if std.compiler is the ideal module for this, but it doesn't seem like any other module is a better fit, and I don't think adding a new module just for this is justifiable.
Note that the example in the changelog entry and the example in the unittest differ - this is due to a limitation of version specifications. They are only permitted at module scope.