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 19645 - Default parameters not checked for @safe #9428

Closed
wants to merge 3 commits into from

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Mar 7, 2019

Default parameters are not checked for @safe/pure/@nogc/nothrow. This is mainly because dmd implemented these checks only for expressions that are in a functions' scope. Before this patch dmd created a normal scope (not function scope) for parameters to be evaluated; this led to default parameters not being checked for @safe/pure/@nogc/nothrow.

To solve the issue I created a mock-up function declaration to trick the compiler that the parameters are in a functions's scope. It would have been ideal to use the initial function declaration for this, but there is no way of obtaining it from a TypeFunction.

nothrow is not handled in this PR as it is a bit different.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
19645 normal Default parameters not checked for @safe

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

@@ -314,6 +314,7 @@ immutable Msgtable[] msgtable =
{ "dup" },
{ "_aaApply" },
{ "_aaApply2" },
{ "__tmpFunc"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to give a bit of a description here.

Copy link
Contributor Author

@RazvanN7 RazvanN7 Mar 7, 2019

Choose a reason for hiding this comment

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

temporary function used as mock-up for the scope to evaluate function parameters ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that now, I might not in 6 months time.

@PetarKirov
Copy link
Member

PetarKirov commented Mar 7, 2019

Shouldn't the function attributes of the default argument expressions be judged based on the caller and not the function declaration?

@jacob-carlborg
Copy link
Contributor

It would have been ideal to use the initial function declaration for this, but there is no way of obtaining it from a TypeFunction.

Why don't you add that?

@@ -1206,6 +1206,10 @@ extern(C++) Type typeSemantic(Type t, Loc loc, Scope* sc)
argsc.stc = 0; // don't inherit storage class
argsc.protection = Prot(Prot.Kind.public_);
argsc.func = null;
Loc tloc = Loc();
auto mockFunc = new FuncDeclaration(tloc, tloc, Id.__tmpFunc, sc.stc, new TypeFunction(ParameterList(), Type.tvoid, LINK.d, sc.stc));
Copy link
Contributor

Choose a reason for hiding this comment

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

Giving the function declaration and identifier will add it to the symbol table which might make it accessible to user code.

@RazvanN7 RazvanN7 closed this Apr 5, 2019
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