Skip to content

analyzer should warn about final super parameters #48699

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

Closed
goderbauer opened this issue Mar 29, 2022 · 12 comments
Closed

analyzer should warn about final super parameters #48699

goderbauer opened this issue Mar 29, 2022 · 12 comments
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead.

Comments

@goderbauer
Copy link
Contributor

goderbauer commented Mar 29, 2022

class Base {
  Base(this.value);
  final int value;
}

class Extended extends Base {
  Extended(final super.value); // note the incorrect final here
}

The analyzer doesn't think anything is wrong with this code, but when I execute it, I get the following error:

Error: Can't have modifier 'final' here.
Try removing 'final'.
  Extended(final super.value);
           ^^^^^

The analyzer should tell me about this problem while I write the code so I can correct it before running my program.

(This was discovered while debugging #48698)

@goderbauer
Copy link
Contributor Author

/cc @pq

@bwilkerson
Copy link
Member

@scheglov

@bwilkerson bwilkerson added the legacy-area-analyzer Use area-devexp instead. label Mar 30, 2022
@scheglov
Copy link
Contributor

Why? Is it specified somewhere?
I see that const, late and var are disallowed.
image

And the super-formal parameters specification also mentions var, super and late.

But in the language specification I see finalConstVarOrType in fieldFormalParameter, so I guess final is allowed.

And this code works fine in VM.

class A {
  final int value;
  A(final this.value);
}

void f(final int value) {}

main() {}

So, I don't see why it should not work for super.name.
@lrhn

@eernstg
Copy link
Member

eernstg commented Mar 30, 2022

A super parameter is already final, so final should be benign, but redundant. I don't see any rules against it.

@pq
Copy link
Member

pq commented Mar 30, 2022

A super parameter is already final, so final should be benign, but redundant

We could definitely consider linting or hinting for this. My preference would be to make it a hint I guess but maybe I'm missing a reason someone might value this redundancy?

@goderbauer
Copy link
Contributor Author

If final is allowed there, why does it give a runtime error? Should that error be removed then as well?

@bwilkerson
Copy link
Member

Yes, it's a bug that you got a runtime error.

It's effectively dead code, so a hint seems reasonable to me.

@pq pq added the devexp-warning Issues with the analyzer's Warning codes label Mar 30, 2022
@eernstg
Copy link
Member

eernstg commented Mar 30, 2022

why does it give a runtime error?

Wouldn't that be a compile-time error from the vm, rather than an actual run time error? If this is true then it should be reported as an issue on the common front end.

@asashour
Copy link
Contributor

asashour commented Mar 31, 2022

I understood there is a new issue to be created for the CFE, while this issue is for the analyzer to have a hint.

I am also under the impression that this should also be applied to FieldFormalParameter, since it has the same behavior.

Please look into https://dart-review.googlesource.com/c/sdk/+/239640

@eernstg
Copy link
Member

eernstg commented Mar 31, 2022

Perhaps the error message from the CFE was emitted by some earlier version? I don't see it with the current CFE, not for a formal of the form final super.x and also not for final this.x.

@asashour
Copy link
Contributor

asashour commented Mar 31, 2022

Or possibly triggered in the last few days in the SDK.

It is reproducible with:

Dart SDK version: 2.17.0-251.0.dev (dev) (Tue Mar 29 00:56:13 2022 -0700) on "windows_x64" (from Flutter master)

@eernstg
Copy link
Member

eernstg commented Mar 31, 2022

Right, I do get the error, with a bleeding edge dart from ac19f52. So it must be a quite recent change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

6 participants