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

Fix issue 22865 - __traits(compiles) affects inferrence of attributes #14144

Merged
merged 1 commit into from May 20, 2022

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented May 19, 2022

DMD is full of variations of:

if (sc.func && !sc.intypeof && !(sc.flags & SCOPE.debug_) && sc.func.setUnsafe(...))

Sometimes intypeof is included, sometimes not. Sometimes SCOPE.debug_ is included, sometimes not.
This fix refactors them to into a single sc.setUnsafe(...), which calls setUnsafe on sc.func. This Scope* version of setUnsafe then does all the checks consistently, including the new sc.flags & SCOPE.compile check for __traits(compiles).

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! 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

Auto-close Bugzilla Severity Description
22865 normal __traits(compiles) affects inferrence of attributes

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

@dkorpel dkorpel force-pushed the setunsafe-speculative-scope branch 3 times, most recently from 195a085 to 46a99e3 Compare May 19, 2022 15:09
@dkorpel dkorpel force-pushed the setunsafe-speculative-scope branch from 46a99e3 to 5314bf7 Compare May 19, 2022 16:40
@dkorpel dkorpel marked this pull request as ready for review May 19, 2022 17:00
@kinke
Copy link
Contributor

kinke commented May 19, 2022

Oh wow, no Buildkite breakage. 👍 - Have you checked all FuncDeclaration.setUnsafe() usages? And will we need something for setImpure() (edit: and setGC()) too?

@dkorpel
Copy link
Contributor Author

dkorpel commented May 20, 2022

Oh wow, no Buildkite breakage.

On the first run, this broke:

https://github.com/atilaneves/automem/blob/7e0aa929c4413679c96adf5f68dc9e4e13b83387/tests/ut/vector.d#L233

Because error() was not called when doing something unsafe in __traits(compiles). Then I added the
if (sc.func.isSafeBypassingInference()) branch that calls error, which fixed it. It's still iffy what is supposed to happen when you do __traits(compiles, unsafe()) in an inferred function: it will now return true, but maybe when you actually insert that code to make it @system, the project as a whole generates an error because it assumes the function is inferred @safe. It's really hard to account for that.

Have you checked all FuncDeclaration.setUnsafe() usages?

Yes

And will we need something for setImpure() (edit: and setGC()) too?

Probably, I haven't looked at it yet.

@RazvanN7 RazvanN7 merged commit 47b1290 into dlang:master May 20, 2022
@dkorpel dkorpel deleted the setunsafe-speculative-scope branch May 20, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants