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

[CP] Disallow final fields to be used in a const context in analyzer #54232

Closed
parlough opened this issue Dec 5, 2023 · 2 comments
Closed

[CP] Disallow final fields to be used in a const context in analyzer #54232

parlough opened this issue Dec 5, 2023 · 2 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. cherry-pick-approved Label for approved cherrypick request merge-to-stable

Comments

@parlough
Copy link
Member

parlough commented Dec 5, 2023

Commit(s) to merge

988c301

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/339820

Issue Description

If a final field is assigned in its declaration (not in a generative constructor) in a class with a constant constructor, constant evaluation incorrectly allows it to be used as a constant. This results in the prefer_const_constructors lint incorrectly suggesting const can be added as well, with no indication that it will result in a compilation error, making it quite easy to get into this situation as it's enabled by default in flutter_lints.

class Test {
  final String fakeConst = '';
  
  const Test();
  
  List<String> te() => const [fakeConst];
                          /// ^^^^^^^^^ No error, but expect non_constant_list_element
}

What is the fix

From @kallentu:

Context: Added a field check in https://dart-review.googlesource.com/c/sdk/+/312347 which the goal back then was to cut down on more unnecessary errors, but this seemed to have caused a regression elsewhere.

Reverting this part of the change and other related tests.

Why cherry-pick

Developers can work around the issue by making some structural adjustments or ignoring the lint, but it may result in confusion from developers and issues uncaught by the analyzer. Also ignoring a lint can result in missing lints in the future.

See an example of the effects of this issue in flutter/devtools#6891.

Risk

low

Issue link(s)

#53927, flutter/devtools#6891

Extra Info

This has a test and fixes a regression in the analyzer's constant context. It doesn't affect the compilers, so issues technically would be caught by those later on or before deployment.

@parlough parlough added the cherry-pick-review Issue that need cherry pick triage to approve label Dec 5, 2023
@lrhn lrhn added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Dec 5, 2023
@athomas
Copy link
Member

athomas commented Dec 5, 2023

Approving based on Brian's comment on the CL.

@athomas athomas added cherry-pick-approved Label for approved cherrypick request merge-to-stable and removed cherry-pick-review Issue that need cherry pick triage to approve labels Dec 5, 2023
copybara-service bot pushed a commit that referenced this issue Dec 5, 2023
…context.

Closes #54232

Bug: #53927
Change-Id: I0774e050c73e677347caad836dd59c9cba71d044
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/333240
Cherry-pick-request: #54232
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339820
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
@athomas
Copy link
Member

athomas commented Dec 6, 2023

Released in 3.2.3.

@athomas athomas closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. cherry-pick-approved Label for approved cherrypick request merge-to-stable
Projects
None yet
Development

No branches or pull requests

6 participants