Improve robustness of comment detection when using flutter analyze --suggestions #172977

reidbaker · 2025-07-30T16:40:16Z

Fixes #172055

~~For some reason a trailing slash at the end of a comment or a line below the commented out line that otherwise would match would cause the regex to match that should be ignored.~~

I could not reason about why the trailing slash would cause a match but the primary change apart from more robust tests is adding ^\s* which pins the match on lines that start with some amount of space before excluding lines that use //

The regex was overbroad at matching and we were getting lucky that the second "version" was the one being selected a trailing slash broke the overbroad match and selected the smaller match that was inside the comment. The new fix was to pin the multi line regex to the start of a line that way we can still handle newlines in the definition while ensuring that commented out lines are not included.

The bug was for AGP version in plugins but the fix was applied to all of the regular expressions in this area of code along with adversarial units tests to prevent regression.

Note for other contributors https://regex101.com/ was a great resource for debugging what was happening here.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

…suggestions

gemini-code-assist

Code Review

The code changes introduce more robust comment detection when using flutter analyze --suggestions. The regexes were updated to pin the match on lines that start with some amount of space before excluding lines that use //. Adversarial unit tests were added to prevent regression.

packages/flutter_tools/lib/src/android/gradle_utils.dart

 // ?<version> is used to name the version group which helps with extraction.
 final _androidGradlePluginRegExpFromId = RegExp(
-  r"""[^\/]*s*id\s*\(?['"]com\.android\.application['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",
+  r"""^\s*[^\/]*s*id\s*\(?['"]com\.android\.application['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",


packages/flutter_tools/lib/src/android/gradle_utils.dart

 // ?<version> is used to name the version group which helps with extraction.
 final _kotlinGradlePluginRegExpFromId = RegExp(
-  r"""[^\/]*s*id\s*\(?['"]org\.jetbrains\.kotlin\.android['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",
+  r"""^\s*[^\/]*s*id\s*\(?['"]org\.jetbrains\.kotlin\.android['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",


ash2moon · 2025-07-30T21:13:44Z

packages/flutter_tools/lib/src/android/gradle_utils.dart

 // ?<version> is used to name the version group which helps with extraction.
 final _kotlinGradlePluginRegExpFromId = RegExp(
-  r"""[^\/]*s*id\s*\(?['"]org\.jetbrains\.kotlin\.android['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",
+  r"""^\s*[^\/]*s*id\s*\(?['"]org\.jetbrains\.kotlin\.android['"]\)?\s+version\s+['"](?<version>\d+(\.\d+){1,2})\)?""",


In the past with super complicated regex, I've tried splitting the regex into multiple lines to group by logic.
Something like this:

final regexParts = [ // Matches id keyword r"""^\s*[^\/]*s*id\s*""", // Matches the plugin identifier r"""\(?['"]org\.jetbrains\.kotlin\.android['"]\)?\s+""", // Matches the version keyword r"""version\s+['"](?<version>\d+(\.\d+){1,2})\)?""", ]; final fullRegex = RegExp(regexParts.join());

Yeah I think that can aid in readability especially for something complicated. It also means there are multiple regex's. You need keep in your head simultaneously and the risk of multiple matches for example on lines that have id in them. That means you need to evaluate multiple matches for each of the regular expressions to find the one you actually care about.

So in this case I think I want to keep these regex's as one line per entry.

Dart doesn't need to use the join, you can just do:

r'''one'''
r'''two'''
r'''three'''

maybe I dont understand the feedback

…paces

lrhn · 2025-08-01T09:16:21Z

packages/flutter_tools/lib/src/android/gradle_utils.dart

 // ?<version> is used to name the version group which helps with extraction.
 final _androidGradlePluginRegExpFromDependencies = RegExp(
-  r"""[^\/]*\s*((\bclasspath\b)|(\bcompileOnly\b))\s*\(?['"]com\.android\.tools\.build:gradle:(?<version>\d+(\.\d+){1,2})\)?""",
+  r"""^[^\/]*\s*((\bclasspath\b)|(\bcompileOnly\b))\s*\(?['"]com\.android\.tools\.build:gradle:(?<version>\d+(\.\d+){1,2})\)?""",


The \ in [^\/] is not needed. It's only needed when used in a JS RegExp expression, /..pattern../, not in the RegExp syntax itself.

You can, and should, use (?:...) instead of (...) if you don't use the capture. (I think the only capture you use is the one named version, so all other (...)s should be (?:...). It's just wasteful to spend time on captures that aren't used.

Not all the parens are needed, fx ((\bclasspath\b)| doesn't need the inner parens at all.

The [^/]* can match anything, including \s* and classpath. Just [^/]*\s* can suffer quadratic backtracking, the rest might make it worse. You can drop the \s*.

The text being looked for is either word classpath or compileOnly, with no / earlier on the same line, followed by optional spaces followed by ' or " quoted string, optionally also parenthesized, where the string has the format com.android.tools.build:gradle: followed by two or three . separated groups of decimal digits.

Should there be a ['"] before the \)??
(It won't matter, the trailing \)? isn't used and doesn't affect matching, so it can just be removed.)

Let me try to simplify that:

r"""\bc(?:lassPath|ompileOnly)\b(?<=^[^/]*)\s*\(?(?<quote>['"])com\.android\.tools\.build:gradle:(?<version>\d+(?:\.d+){1,2})\k<quote>""";

(I expect a regexp compiler to be smart enough to move the c out of (classPath|compileOnly), but I did it manually just to be explicit.)

This searches for classPath and compileOnly first, only then checks if there is no / before them.
That should give much fewer hits than starting by looking for / on each line, and then backtracking until maybe finding classpath|compileOnly.

It is probably just as efficient to do (?<=^[^/]*)\b(?:classpath|compileOnly)\b...
Maybe more, if the regexp compiler is not smart enough to recognize that it can skip past "classpath" on the way back. Maybe less if it starts scanning every line instead of looking for classpath|compileOnly first.

Thank you I took most of this feedback. Places where I deviated were in the extracted c from classpath and compileOnly and the trailing quote needed to have some additional matching because -beta3 is a valid suffix (caught by tests).

packages/flutter_tools/lib/src/android/gradle_utils.dart

lrhn · 2025-08-01T09:29:05Z

packages/flutter_tools/lib/src/android/gradle_utils.dart

 // Groovy DSL with single quotes - 'com.android.tools.build:gradle:{{agpVersion}}'
 // Groovy DSL with double quotes - "com.android.tools.build:gradle:{{agpVersion}}"
 // Kotlin DSL - ("com.android.tools.build.gradle:{{agpVersion}}")
 // ?<version> is used to name the version group which helps with extraction.


Consider documenting the entire thing being matched, including why the leading [^/]*\s* is there.
(You can't debug a RegExp if you don't know, without looking at the RegExp, what it should match.)

…use a look behind for commented out lines based on code reivew feedback

auto-submit · 2025-08-01T18:45:14Z

autosubmit label was removed for flutter/flutter/172977, because - The status or check suite Linux tool_integration_tests_6_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

…plugin

…analyze --suggestions (flutter/flutter#172977)

Roll Flutter from 871849e4b6bf to beda687d63f2 (34 revisions) flutter/flutter@871849e...beda687 2025-08-04 jason-simmons@users.noreply.github.com [Impeller] Improvements to the Vulkan pipeline cache data writer (flutter/flutter#173014) 2025-08-04 jason-simmons@users.noreply.github.com [Impeller] Terminate the fence waiter but do not reset it during ContextVK shutdown (flutter/flutter#173085) 2025-08-04 matt.kosarek@canonical.com Add the 'windowing' feature flag and use to wrap an implementation for regular windows that always throws (flutter/flutter#172478) 2025-08-04 30870216+gaaclarke@users.noreply.github.com licenses_cpp: reland switch 4 (flutter/flutter#173139) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from 763bba9c33fd to dce9550a1356 (1 revision) (flutter/flutter#173219) 2025-08-04 1063596+reidbaker@users.noreply.github.com Improve robustness of comment detection when using flutter analyze --suggestions (flutter/flutter#172977) 2025-08-04 ahmedsameha1@gmail.com Make sure that a LicensePage doesn't crash in 0x0 environment (flutter/flutter#172610) 2025-08-04 engine-flutter-autoroll@skia.org Roll Packages from f0645d8 to 1a72287 (4 revisions) (flutter/flutter#173215) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from edf0f8a5bba6 to 763bba9c33fd (1 revision) (flutter/flutter#173213) 2025-08-04 sokolovskyi.konstantin@gmail.com [web] Add Intl.Locale to parse browser languages. (flutter/flutter#172964) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from 439f80973f4a to edf0f8a5bba6 (2 revisions) (flutter/flutter#173204) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from 9d30cc96a3b2 to 439f80973f4a (1 revision) (flutter/flutter#173201) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from 39c70f883c0e to 9d30cc96a3b2 (1 revision) (flutter/flutter#173197) 2025-08-04 engine-flutter-autoroll@skia.org Roll Skia from 09667386bcba to 39c70f883c0e (1 revision) (flutter/flutter#173194) 2025-08-04 codefu@google.com fix: get content hash for master on local engine branches (third attempt) (flutter/flutter#173169) 2025-08-03 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from wZuA8Dqty4scQkZuV... to ufssK8EgJ_9RpLFgu... (flutter/flutter#173190) 2025-08-03 engine-flutter-autoroll@skia.org Roll Skia from e056e47e118b to 09667386bcba (2 revisions) (flutter/flutter#173185) 2025-08-03 codefu@google.com fix: tag fuchsia package after uploading (flutter/flutter#173140) 2025-08-02 engine-flutter-autoroll@skia.org Roll Skia from 7ef888e30a28 to e056e47e118b (1 revision) (flutter/flutter#173170) 2025-08-02 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from yFLr81lLXmSX7n11D... to wZuA8Dqty4scQkZuV... (flutter/flutter#173166) 2025-08-02 engine-flutter-autoroll@skia.org Roll Skia from af6d6eb383a6 to 7ef888e30a28 (1 revision) (flutter/flutter#173157) 2025-08-02 32538273+ValentinVignal@users.noreply.github.com Add `innerRadius` to `RadioThemeData` (flutter/flutter#173120) 2025-08-02 engine-flutter-autoroll@skia.org Roll Skia from 81dd6aa516b0 to af6d6eb383a6 (2 revisions) (flutter/flutter#173144) 2025-08-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "fix: get content hash for master on local engine branches (#173114)" (flutter/flutter#173145) 2025-08-01 engine-flutter-autoroll@skia.org Roll Dart SDK from 6e1bb159c860 to 3f1307d72d6f (2 revisions) (flutter/flutter#173136) 2025-08-01 engine-flutter-autoroll@skia.org Roll Skia from 9e2c59634961 to 81dd6aa516b0 (1 revision) (flutter/flutter#173135) 2025-08-01 737941+loic-sharma@users.noreply.github.com Reformat text.dart's code snippets (flutter/flutter#172976) 2025-08-01 dmgr@google.com experiment with docs properties (flutter/flutter#173124) 2025-08-01 codefu@google.com fix: get content hash for master on local engine branches (flutter/flutter#173114) 2025-08-01 ahmedsameha1@gmail.com Make sure that a RawAutocomplete doesn't crash in 0x0 environment (flutter/flutter#172812) 2025-08-01 bungeman@chromium.org Add skia_ports_fontmgr_android_parser_sources (flutter/flutter#172979) 2025-08-01 engine-flutter-autoroll@skia.org Roll Skia from 49e39cd3cf18 to 9e2c59634961 (10 revisions) (flutter/flutter#173125) 2025-08-01 72062416+Yash-Dhrangdhariya@users.noreply.github.com fix: 🐛 Add equality and hashCode implementations to ResizeImage (flutter/flutter#172643) 2025-08-01 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from xo_bqOoFuBKFmgKxn... to yFLr81lLXmSX7n11D... (flutter/flutter#173117) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...

…suggestions (flutter#172977) Fixes flutter#172055 ~For some reason a trailing slash at the end of a comment or a line below the commented out line that otherwise would match would cause the regex to match that should be ignored.~ ~I could not reason about why the trailing slash would cause a match but the primary change apart from more robust tests is adding `^\s*` which pins the match on lines that start with some amount of space before excluding lines that use `//`~ The regex was overbroad at matching and we were getting lucky that the second "version" was the one being selected a trailing slash broke the overbroad match and selected the smaller match that was inside the comment. The new fix was to pin the multi line regex to the start of a line that way we can still handle newlines in the definition while ensuring that commented out lines are not included. The bug was for AGP version in plugins but the fix was applied to all of the regular expressions in this area of code along with adversarial units tests to prevent regression. Note for other contributors https://regex101.com/ was a great resource for debugging what was happening here. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.

…analyze --suggestions (flutter/flutter#172977)

Improve robustness of comment detection when using flutter analyze --…

27f82ad

…suggestions

reidbaker requested a review from jtmcdole July 30, 2025 16:40

reidbaker requested a review from a team as a code owner July 30, 2025 16:40

github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. team-android Owned by Android platform team labels Jul 30, 2025

gemini-code-assist bot reviewed Jul 30, 2025

View reviewed changes

Formatting

7aa8e38

ash2moon reviewed Jul 30, 2025

View reviewed changes

reidbaker and others added 2 commits July 31, 2025 10:28

remove s* that was a mistake and match to beginning of line without s…

c86ecca

…paces

Merge branch 'master' into i172055-trailing-slash-match

e48d3b2

jtmcdole approved these changes Jul 31, 2025

View reviewed changes

lrhn reviewed Aug 1, 2025

View reviewed changes

reidbaker and others added 3 commits August 1, 2025 10:52

rework the beginning of each regular experssion to match a word then …

5cb1492

…use a look behind for commented out lines based on code reivew feedback

shorter digit matching for gradle versions

ef3c9d0

Merge branch 'master' into i172055-trailing-slash-match

38b2d01

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2025

reidbaker requested a review from ash2moon August 1, 2025 18:36

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2025

reidbaker and others added 2 commits August 4, 2025 10:39

Add test for mising case groovy dsl, kotlin applied in settings from …

52e1623

…plugin

Merge branch 'master' into i172055-trailing-slash-match

4438649

reidbaker requested a review from jtmcdole August 4, 2025 16:33

gmackall approved these changes Aug 4, 2025

View reviewed changes

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 4, 2025

auto-submit bot added this pull request to the merge queue Aug 4, 2025

Merged via the queue into flutter:master with commit 561c3d2 Aug 4, 2025
141 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 4, 2025

engine-flutter-autoroll mentioned this pull request Aug 4, 2025

Roll Flutter from 871849e4b6bf to bd542bb68758 (31 revisions) flutter/packages#9743

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025

561c3d27d Improve robustness of comment detection when using flutter …

73008ca

…analyze --suggestions (flutter/flutter#172977)

engine-flutter-autoroll mentioned this pull request Aug 4, 2025

Roll Flutter from 871849e4b6bf to beda687d63f2 (34 revisions) flutter/packages#9745

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 4, 2025

561c3d27d Improve robustness of comment detection when using flutter …

d60a819

…analyze --suggestions (flutter/flutter#172977)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025

561c3d27d Improve robustness of comment detection when using flutter …

2997707

…analyze --suggestions (flutter/flutter#172977)

Improve robustness of comment detection when using flutter analyze --suggestions #172977

Improve robustness of comment detection when using flutter analyze --suggestions #172977

Uh oh!

Conversation

reidbaker commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

ash2moon Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

reidbaker Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtmcdole Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

reidbaker Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

lrhn Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

reidbaker Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrhn Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

reidbaker commented Jul 30, 2025 •

edited

Loading

reidbaker Jul 31, 2025 •

edited

Loading