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 15379 - "final" attribute on function parameter #3354

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Jul 24, 2022

final is not allowed here in D2.

void foo(final int a){} // error!
finalpar.d(1): Error: variable `finalpar.foo.a` cannot be `final`, perhaps you meant `const`?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ntrel! 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
15379 normal "final" attribute on function parameter

@dkorpel
Copy link
Contributor

dkorpel commented Jul 24, 2022

It's not a parse error though, this compiles:

version(none) void foo(final int a) {}

So the grammar should allow it.

@ljmf00
Copy link
Member

ljmf00 commented Jul 24, 2022

It's not a parse error though, this compiles:

version(none) void foo(final int a) {}

So the grammar should allow it.

Why we should allow something that apparently doesn't have a useful meaning? I think moving the error to the parser, if we can have the same kind of error message, and fixing the grammar is a good path. Otherwise, if this has some meaning for templates, we should perhaps update the spec?

@dkorpel
Copy link
Contributor

dkorpel commented Jul 24, 2022

Why we should allow something that apparently doesn't have a useful meaning?

I meant to say "the grammar should allow it or the implementation should be updated to match the new grammar".

I think moving the error to the parser, if we can have the same kind of error message, and fixing the grammar is a good path.

There are plenty of errors that could be caught by the parser but are currently caught during semantic analysis, keeping the grammar simpler.

version(none) 
void f() scope {
    break;
    default:
    switch (0) {}
    "useless";
}

I'm not against changing the parser here per se, but moving errors to parse time isn't a "good path" by default.

@ntrel
Copy link
Contributor Author

ntrel commented Jul 25, 2022

There are plenty of errors that could be caught by the parser but are currently caught during semantic analysis, keeping the grammar simpler.

In this case the grammar is not simpler. I think the reason it parses was for the error advising to use const. That error is probably not needed anymore, or could perhaps be done in the parser. If not, the spec should note that final is not valid here.

@ntrel
Copy link
Contributor Author

ntrel commented Jul 25, 2022

I think the reason it parses was for the error advising to use const

Perhaps it was so D1 and D2 code can co-exist in the same module? So perhaps it should still parse if this is still a goal.

@ntrel
Copy link
Contributor Author

ntrel commented Jul 27, 2022

As this seems intentional to not error on parsing, I have added a note to the docs instead of removing final.

@dkorpel Can you remove the waiting for dmd label now please?

Note that `final` is a semantic error in D2, but not a parse error.
```d
void foo(final int a){} // error!
```
```
finalpar.d(1): Error: variable `finalpar.foo.a` cannot be `final`, perhaps you meant `const`?
```
@RazvanN7 RazvanN7 merged commit 6a4cda2 into dlang:master Jul 29, 2022
@ntrel ntrel deleted the patch-5 branch July 30, 2022 10:20
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