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] bump linter to fix crashes / records false positives #52185

Closed
pq opened this issue Apr 26, 2023 · 12 comments
Closed

[CP] bump linter to fix crashes / records false positives #52185

pq opened this issue Apr 26, 2023 · 12 comments
Assignees
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-beta

Comments

@pq
Copy link
Member

pq commented Apr 26, 2023

Commit(s) to merge

92b963ec83b2b9a3ca40d08a67382d5a1644a37d

Target

beta

Prepared changelist for beta/stable

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

Issue Description

Fixes issues w/ patterns/records:

  • crashes in use_build_context_synchronously
  • false positives in non_constant_identifier_names
  • false positives in no_leading_underscores_for_local_identifiers
  • false positives in constant_identifier_names
  • false positives in prefer_final_locals

What is the fix

See linked fixes.

This bumps to the version of the linter currently in Google3.

We elected to do this rather than a series of targeted cherry picks because:

  1. we moved to Dart 3.0 since the linter in stable making simple cherry picks hard and
  2. an aggregate branch on the linter that collected the required changes would be complex

Since this linter is currently in Google3 and analytics have not shown any new crashes and we haven't received any new bug reports we feel this is safer than the alternative.

Why cherry-pick

To fix crashes and fix recommended lint rule over-reporting for 3.0 code using patterns.

Risk

low

Issue link(s)

dart-lang/linter#4279, #59120, #59121, #59122, #59123

Extra Info

Full list of changes since version in stable:

e36813b fix non_constant_identifier_names pattern field over-reporting (dart-lang/linter#4300)
7050946 fix constant_identifier_names pattern field over-reporting (dart-lang/linter#4298)
ae3501b fix no_leading_underscores_for_local_identifiers pattern field over-reporting (v4296)
17dcb46 prefer_final_in_for_each_test for final records (dart-lang/linter#4294)
f38c72e tests for prefer_final_in_for_each pattern support (dart-lang/linter#4291)
a7facd7 tidy up formal parameter inspection (dart-lang/linter#4289)
0c18a65 unnecessary_final support for ForEachPartsWithPatterns (dart-lang/linter#4288)
2212d95 fix prefer_final_locals false positive in foreach loops (dart-lang/linter#4287)
6646b99 fix Dart3 unsafe mixing in (dart-lang/linter#4285)
e48f286 Move use_key_in_widget_constructors tests to be reflective (dart-lang/linter#4283)
82e7147 Fix unrelated_type_equality_checks when comparing enums (dart-lang/linter#4282)
d8ccf81 fix pr labeler yaqml indent (dart-lang/linter#4051)
ca2a9fe Lint about comparing booleans to boolean literals (dart-lang/linter#3973)
4c2af04 Fix false negative in prefer_contains (dart-lang/linter#2830)
efa4c97 Update releasing.md with more pre-release steps (dart-lang/linter#3901)
a8c903c bump cli_util dep (dart-lang/linter#4206)
500c2ce New rule: Use matching super parameter names (dart-lang/linter#4263)
e22fdae [use_build_context_synchronously] Fix for unprotected 'last' invocation. (dart-lang/linter#4279)
f9edf66 bump gh pages build to dev SDK (dart-lang/linter#4275)
e944fc7 hasLeadingUnderscore extension cleanup (dart-lang/linter#4274)
676c711 inverted token type extension (dart-lang/linter#4272)
686e85d => Dart 3.0! (dart-lang/linter#4267)
74fa5ff Add comments to use_build_context_synchronously tests with fuller explanations (dart-lang/linter#4271)
a215513 prefer_const_constructors_in_immutables: simplify visitConstructorDeclaration override (dart-lang/linter#4254)
eb0ca50 Account for records in type relatedness checks (dart-lang/linter#4266)
0b4787f stop pana baseline checking (dart-lang/linter#4269)
d9c05ab Move more use_build_context_synchronously tests (dart-lang/linter#4264)
fffb74c Move several if-concerned use_build_context_synchronously tests (dart-lang/linter#4255)

Note that the unrelated changes were deemed low-risk.

@pq pq added the cherry-pick-review Issue that need cherry pick triage to approve label Apr 26, 2023
@mit-mit
Copy link
Member

mit-mit commented Apr 26, 2023

SGTM

@itsjustkevin
Copy link
Contributor

@pq in the case that this ends up causing issues, would there be any concerns with simply rolling back the revision?

@pq
Copy link
Member Author

pq commented Apr 26, 2023

in the case that this ends up causing issues, would there be any concerns with simply rolling back the revision?

Nope. I think that would be the right thing to do. If there are issues we can just go back to what's in beta currently and tough it out until we can put together something for the next stable release.

@itsjustkevin
Copy link
Contributor

@jacob314 and @vsmenon could you take a look at this cherry-pick request?

@srawlins
Copy link
Member

srawlins commented Apr 27, 2023

I think another commit in the list above is also a good reason to cherry-pick:

eb0ca50 Account for records in type relatedness checks (dart-lang/linter#4266)

And a good chunk of the commits in the list are zero risk in that they only touch tests or doc files like README.md.

@vsmenon
Copy link
Member

vsmenon commented Apr 27, 2023

@athomas @leafpetersen - thoughts? We're getting close enough to the release - the bar is high. Are we better off holding this for the next dot release?

@leafpetersen
Copy link
Member

@pq can you summarize how common these issue are? Is this fixing something that many people are likely to run into, or is this more of a "now and then" sort of thing? We have a patch release going out ~1 week after stable, is it reasonable to push this out or is this something that's going to significantly impact the experience of a significant number of users when they try things out?

@pq
Copy link
Member Author

pq commented May 1, 2023

@scheglov: can you speak to the frequency of the crash you fixed? (It's in a flutter recommended lint so could potentially affect a lot of 3P folks.)

A few of the false positives are in recommended lints and will trigger with records in particular though those are easier to (temporarily) ignore.

@scheglov
Copy link
Contributor

scheglov commented May 1, 2023

High enough that I noticed it looking through crash reports.
I don't know enough to make good query, but probably using the existing crash UI this.
Currently it shows 751 crashes from 85 users that used beta.

@srawlins
Copy link
Member

srawlins commented May 1, 2023

One more note: we definitely saw the crashes drop after the fix made it to the Dart SDK. Crashes in general are reduced as well (we did not create any worse problem with the fix :D )

@itsjustkevin itsjustkevin added merge-to-beta cherry-pick-approved Label for approved cherrypick request labels May 1, 2023
@itsjustkevin
Copy link
Contributor

@pq and @srawlins assuming the failures here are expected we can merge this one.

@pq
Copy link
Member Author

pq commented May 1, 2023

Yeah. Those failures are unrelated.

@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label May 1, 2023
@mraleph mraleph added the area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-beta
Projects
None yet
Development

No branches or pull requests

10 participants