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

Implement DIP1035 - system variables #14478

Merged
merged 2 commits into from
Oct 14, 2022
Merged

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Sep 23, 2022

Make access of variables with STC.system a deprecation/error depending on -preview=systemVariables.
Does not implement inference of @system based on initializers yet, or checking against void initializing / array casting.

@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

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

@@ -542,6 +542,11 @@ extern (C++) abstract class Declaration : Dsymbol
return (storage_class & STC.future) != 0;
}

final extern(D) bool isSystem() const pure nothrow @nogc @safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's extern(D), do you think it should be exposed to the extern(C++) interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I missed that. Thats a question for @ibuclaw or @kinke

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, extern(D) does not need to be exposed to other backends.

@dkorpel
Copy link
Contributor Author

dkorpel commented Sep 23, 2022

It's currently red because of deprecations when compiling druntime/phobos caused by dlang/druntime#3007 which paints not only functions now, but also constants and variables @system.

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

A list of suggestions and questions:

  • declaring @System variable in a while/if condition and then using it should be tested.
  • declaring @System parameters for functions should be tested
  • is it allowed to take the address of a @System variable in @safe code? Anyway, a test with this should be added.
  • is it allowed to pass a @System variable to a @safe function?
  • should we add a trait similar to getFunctionAttributes which gets the attributes of a variable since we now have attributes for variables?
  • if you can declare a system parameter, is it possible to declare a template function that receives a system parameter?

@@ -22,7 +22,6 @@ version (linux):
extern (C):
nothrow:
@nogc:
@system:
Copy link
Contributor

Choose a reason for hiding this comment

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

A note on this: if we ever transition to @safe-by-default the functions that are present in this file will be wrongfully considered as being @safe. Maybe add the @System to those functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

This may apply to some of the other files as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember DIP1028 and what happened to it?

Change my mind

I don't think I have to account for that anymore.

@dkorpel
Copy link
Contributor Author

dkorpel commented Sep 28, 2022

  • declaring @System variable in a while/if condition and then using it should be tested.

Not allowed by the grammar

  • declaring @System parameters for functions should be tested

Not allowed by the grammar

  • is it allowed to take the address of a @System variable in @safe code?

No, then you access it

Anyway, a test with this should be added.

Okay

  • is it allowed to pass a @System variable to a @safe function?

Only if you're calling it from a @system or @trusted function

  • should we add a trait similar to getFunctionAttributes which gets the attributes of a variable since we now have attributes for variables?

Probably, but not in this PR

  • if you can declare a system parameter, is it possible to declare a template function that receives a system parameter?

You can't, and you can't

@dkorpel
Copy link
Contributor Author

dkorpel commented Sep 30, 2022

@RazvanN7 Anything else?

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Looks good to me, however, I would prefer if @WalterBright merges this.

@WalterBright WalterBright merged commit 1188adc into dlang:master Oct 14, 2022
@dkorpel dkorpel deleted the systemvariables branch October 14, 2022 07:04
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.

5 participants