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

Refactor -transition and -dip to -transition, -revert and -preview #9206

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Jan 5, 2019

  • the old -dip flags won't be removed nor will using them trigger a deprecation
  • as the dips have been approved they are essentially transitions (which is why there's a -transition=noDIP25 switch already)
  • Technically, dip1008 hasn't been fully approved yet, but as it's similar to -transition=markdown imho it belongs in -transition to. However, an argument could be made that there should be an -experiment or -unstable flag for such experimental features.

Motivation:

  • we could add a -std=d20 (or similar flag) which will enable a bunch of these transitions by default
  • all transitions are equal, but currently some are more equal
  • by documenting them as -transition we unify the user interface and also show the clear intent that they will be the default eventually

I could very easily add tests like in #9205 (though I would prefer to append to this file).

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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 fetch digger
dub run digger -- build "master + dmd#9206"

@WalterBright
Copy link
Member

by documenting them as -transition we unify the user interface and also show the clear intent that they will be the default eventually

-transition should be for reverting to previous behavior, not for trying out new behavior. That's why -dip25 tried out the new behavior, and -transition=noDIP25 reverted it.

Unfortunately, things are a bit inconsistent at the moment.

@wilzbach
Copy link
Member Author

wilzbach commented Jan 6, 2019

Unfortunately, things are a bit inconsistent at the moment.

Hmm, how about adding a new -nightly switch that then contains all upcoming features/modifications?
Though looking at the list of current transition most of them are "upcoming":

Language changes listed by -transition=id:
  =all              list information on all language changes
  =field,3449       list all non-mutable fields which occupy an object instance
  =import,10378     revert to single phase name lookup
  =dtorfields,14246 destruct fields of partially constructed objects
  =checkimports     give deprecation messages about 10378 anomalies
  =complex,14488    give deprecation messages about all usages of complex or imaginary types
  =intpromote,16997 fix integral promotions for unary + - ~ operators
  =tls              list all variables going into thread local storage
  =fixAliasThis     when a symbol is resolved, check alias this scope before going to upper scopes
  =markdown         enable Markdown replacements in Ddoc
  =vmarkdown        list instances of Markdown replacements in Ddoc

We really need to find a good & managed model to enable transitions. Most of these transition flag have been there for many years ...

@PetarKirov
Copy link
Member

How about -std=2019?

@WalterBright
Copy link
Member

@wilzbach you have a good point. An idea:

  1. only document -transition features that are informational, like -transition=vmarkdown
  2. add -preview= for upcoming features, i.e. -preview=markdown
  3. add -revert= for reverting to old behaviors, i.e. -revert=dip25

@wilzbach wilzbach changed the title Move standalone -dip flags to -transition Refactor -transition and -dip to -transition, -revert and -preview Jan 7, 2019
@wilzbach wilzbach force-pushed the transition-dip branch 3 times, most recently from 9358e5e to 8cdb927 Compare January 7, 2019 16:31
src/dmd/mars.d Outdated
buf ~= `params.`~t.paramName~` = true;`;
buf ~= "break;";

foreach (t; features)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe sth. like:

Suggested change
foreach (t; features)
foreach (t; mixin("Usage."~name~"s"))

Though this would make it even more assumptive.

@wilzbach
Copy link
Member Author

wilzbach commented Jan 7, 2019

An idea: ...

I really like the idea and went ahead and implemented it. All old flags have been kept and will continue to work (even without a deprecation or warning message).
Though a few questions:

  1. Should we automatically generate a -revert flag for each -preview flag?

Even if we enable a -preview by default, people might have been using -preview=xyz in their build scripts, so we would have to keep the flag around anyhow for a few releases (though we could at least remove it from the documentation when we flip it to default).

  1. Should we use this chance to rename some of the flags (or add aliases)?

I think for most non-DMD developers -preview=nogc is a lot more descriptive than a cryptic number Or should we support two aliases here?

With the same motivation, I didn't keep the keep bugzilla numbers when moving the flags to -preview (e.g. what does -preview=14246 mean to you when you see it?). Though as mentioned above, these flags still continue to work without breakage.

src/dmd/mars.d Outdated
if (*ps != '=')
return false;

if (strcmp(ps + 1, "?") == 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use isHelpOption once #9205 has been merged.

@wilzbach
Copy link
Member Author

wilzbach commented Jan 7, 2019

How about -std=2019?

I think we're too late for that, but we could have -std=20 (or -std=2020). Though most of the currently experimental features aren't ready for prime-time yet and most of them should probably be changed to emit deprecations, s.t. upgrading is more seamless for users.

That being said, here's what a -std=2020 switch could already do:

  • -checkaction=context (assert messages with context)
  • -verrors=context (error messages with context)
  • -preview=markdown

Though all of these can be simply enabled by default after a few months of testing in the wild. The "good stuff" like fixAliasThis would create a split between libraries like Python 2/3.

@wilzbach
Copy link
Member Author

As #9217 refactors the way CLI options with arguments and usage pages are handled, we should probably wait with this until that one has been merged -> setting Blocked on this for now. Though this doesn't mean that we can't already reach a consensus on whether all -preview flags should automatically also exist as -revert flags.

src/dmd/cli.d Outdated Show resolved Hide resolved
src/dmd/cli.d Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member

Blocking #9331

@WalterBright WalterBright added the Review:Blocking Other Work review and pulling should be a priority label Feb 8, 2019
@wilzbach
Copy link
Member Author

wilzbach commented Feb 8, 2019

Blocking #9331

What is this PR blocked on? It's passing all CIs, no?
In other words, what can/should I do to unblock this PR?

@thewilsonator
Copy link
Contributor

What is this PR blocked on?

Nothing, this is blocking something else.

@wilzbach
Copy link
Member Author

wilzbach commented Feb 8, 2019

Nothing, this is blocking something else.

Yeah, that's my take too, so why not merge it and unblock the other PR then?

@WalterBright
Copy link
Member

At the time I wrote that, this PR was not passing everything. I see that it is now.

@WalterBright WalterBright merged commit b58f36d into dlang:master Feb 8, 2019
@wilzbach wilzbach deleted the transition-dip branch February 9, 2019 00:49
@wilzbach
Copy link
Member Author

wilzbach commented Feb 9, 2019

After thinking a bit about it, I realized that we should probably provide a changelog entry for this -> #9336

@UplinkCoder
Copy link
Member

Phobos in the compiler? @wilzbach why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:auto-merge Review:Blocking Other Work review and pulling should be a priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants