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

Enable -preview=in by default in the standard library #14557

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Oct 14, 2022

This builds on the idea I mentioned in the past, of having pragma(preview, "feature") in modules.
However, I was never really found of the idea, as it risked balkanizing the language. Walter shared similar concerns.
So the approach this takes is to allow compiler devs to implement a feature similar to pragma(preview) without exposing it to the user.

While doing so, I originally had to fight to make it work for the scopeness, because those changes are much more invasive. Separating the two concerns reduces the patch to almost nothing, so I think that's a clear win.

Let's see what the CI has to say.

Because the standard library (Phobos / druntime) are distributed compiled,
we have a recurent problem with build flags: User-supplied build flags,
such as `-version`, `-{debug,release}`, or `-preview` have no effect on
the generated code, but may affect semantic analysis.

This was particularly problematic when DIP1000 was implemented,
as it originally changed the mangling, leading to years where the switch
was unusable. This new approach attempts to circumvent the problem
by having scope-specific flags which will allow turning on a feature
in the standard library independently of what user code does.

Currently, this has no effect: When `-preview=in` was implemented,
all places where it could turn into `ref` were stripped, to prevent
the issue from creeping early adopters. However, in the short term,
it could be used in concert with `-checkaction=context`,
allowing to reduce template instantiation and bloat.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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#14557"

@thewilsonator
Copy link
Contributor

I like the idea, i'm slightly concerned about templates and their modules of origin. I remember it took me a while to figure out how to do something similar for dcompute.

@Geod24
Copy link
Member Author

Geod24 commented Oct 14, 2022

dget.d(113): Error: function std.net.curl.HTTP.onReceiveHeader(void delegate(in char[] key, in char[] value) callback) is not callable using argument types (void)

So the good thing is that this works as intended: The compiler correctly detects that the delegate types are not covariant. The bad thing is that it's likely to trigger in user code as well. I need to see how to handle this.

@Geod24
Copy link
Member Author

Geod24 commented Oct 14, 2022

    http.onReceiveHeader((k, v)
    {
        if (k == "content-length")
            contentLength = to!size_t(v);
    });

Actually in that case it's just missing inference.

@Geod24
Copy link
Member Author

Geod24 commented Oct 14, 2022

Depends on dlang/phobos#8601 for Windows build

@thewilsonator
Copy link
Contributor

Dependant PR merged

@nordlow
Copy link
Contributor

nordlow commented Oct 16, 2022

Needs header regeneration via

../../generated/build cxx-headers-test AUTO_UPDATE=1

? See https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=5363935&isPull=true.

@Geod24
Copy link
Member Author

Geod24 commented Oct 16, 2022

The next step would be to get inference of STC working. Which is likely to be a big undertaking, considering the mess that it currently is. So this PR is blocked until that is done. I will submit the independent commits in their own PRs.

This somewhat simplifies the code, and decouple DIP1000 behavior from
'-preview=in' behavior. The '-preview=in' specific behavior is now
limited to the 'ref' / rvalue feature.
Because the standard library (Phobos / druntime) are distributed compiled,
we have a recurent problem with build flags: User-supplied build flags,
such as `-version`, `-{debug,release}`, or `-preview` have no effect on
the generated code, but may affect semantic analysis.

This was particularly problematic when DIP1000 was implemented,
as it originally changed the mangling, leading to years where the switch
was unusable. This new approach attempts to circumvent the problem
by having scope-specific flags which will allow turning on a feature
in the standard library independently of what user code does.

Currently, this has no effect: When `-preview=in` was implemented,
all places where it could turn into `ref` were stripped, to prevent
the issue from creeping early adopters. However, in the short term,
it could be used in concert with `-checkaction=context`,
allowing to reduce template instantiation and bloat.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants