Skip to content

[pigeon] Fix basePath resolution when paths share a string prefix with basePath - #13342

Open
tarrinneal wants to merge 5 commits into
flutter:mainfrom
tarrinneal:fix-base-path-prefix
Open

tarrinneal wants to merge 5 commits into
flutter:mainfrom
tarrinneal:fix-base-path-prefix

Conversation

@tarrinneal

Copy link
Copy Markdown
Contributor

Replaces string-prefix !....startsWith(basePath) checks across ffigen_config_generator.dart, jnigen_config_generator.dart, pigeon_lib.dart, and pigeon_lib_internal.dart with a path-aware isWithinOrEquals(basePath, ...) helper using path.posix.equals and path.posix.isWithin.

Previously, when basePath was set (e.g. basePath: 'app') and an output or directory path happened to start with the same string prefix without being inside basePath (e.g. dartOut: 'app_bindings/lib/src/messages.g.dart'), .startsWith(basePath) evaluated to true and skipped prepending basePath, while _getFile unconditionally joined basePath and output.

Follow-up from #13283 (review).

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [versioning exemption] this PR falls under[^1].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [changelog exemption] this PR falls under[^1].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Oct 20, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new helper function isWithinOrEquals in generator_tools.dart to resolve issues where basePath is incorrectly matched when an output or directory path shares a string prefix with it. This helper replaces startsWith checks across multiple generators, and corresponding unit tests are added. The reviewer suggests explicitly checking that internalOptions.basePath is not empty before calling isWithinOrEquals in pigeon_lib.dart to avoid fragile behavior with empty strings.

Comment on lines 911 to +914
(internalOptions.basePath != null &&
targetConfigDir != null &&
targetConfigDir.isNotEmpty &&
!targetConfigDir.startsWith(internalOptions.basePath!))
!isWithinOrEquals(internalOptions.basePath!, targetConfigDir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure consistency with other generators and avoid relying on fragile behavior of path.isWithin when called with an empty string, we should explicitly check that internalOptions.basePath is not empty before calling isWithinOrEquals.

            (internalOptions.basePath != null &&
                internalOptions.basePath!.isNotEmpty &&
                targetConfigDir != null &&
                targetConfigDir.isNotEmpty &&
                !isWithinOrEquals(internalOptions.basePath!, targetConfigDir))

Comment on lines 934 to +937
(internalOptions.basePath != null &&
targetConfigDir != null &&
targetConfigDir.isNotEmpty &&
!targetConfigDir.startsWith(internalOptions.basePath!))
!isWithinOrEquals(internalOptions.basePath!, targetConfigDir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure consistency with other generators and avoid relying on fragile behavior of path.isWithin when called with an empty string, we should explicitly check that internalOptions.basePath is not empty before calling isWithinOrEquals.

            (internalOptions.basePath != null &&
                internalOptions.basePath!.isNotEmpty &&
                targetConfigDir != null &&
                targetConfigDir.isNotEmpty &&
                !isWithinOrEquals(internalOptions.basePath!, targetConfigDir))

@davidmorgan davidmorgan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this fixes the app_bindings case. One question about the approach, and one pre-existing issue nearby.

Output paths (dartOut, swiftOut) are never pre-joined with basePath, and _getFile always writes to path.posix.join(basePath, out). So the config is correct exactly when it also uses the plain join, which already returns out unchanged when out is absolute. With the "already inside" skip, a relative output that really is under basePath still ends up wrong, before and after this PR. For example, with basePath: 'app' and dartOut: 'app/lib/x.dart', the file is written to app/app/lib/x.dart but the config points at app/lib/x.dart. The check is only needed for the top-level appDirectory/configDirectory, which InternalPigeonOptions pre-joins.

Pre-existing, not from this PR: the per-language swiftOptions.configDirectory and kotlinOptions.configDirectory aren't pre-joined. shouldGenerate writes the config file under them without basePath, but pigeon_lib.dart runs ffigen/jnigen in join(basePath, configDirectory). So with basePath set, the config file is written in one place and the tool runs in another.

(basePath.isNotEmpty && swiftOutPath.isNotEmpty && !swiftOutPath.startsWith(basePath))
(basePath.isNotEmpty &&
swiftOutPath.isNotEmpty &&
!isWithinOrEquals(basePath, swiftOutPath))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For output paths, is the skip needed at all? _getFile always joins, so path.posix.join(basePath, swiftOutPath) would match the written file in every case, absolute paths included. As written, swiftOut: 'app/Messages.g.swift' under basePath: 'app' is still mismatched. And ./app/Messages.g.swift was joined correctly before, but is now treated as already inside, so the config path doesn't match the written file. Same for dartOutPath below and in jnigen_config_generator.dart.

@davidmorgan davidmorgan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more site with the same prefix bug, missed in the merge with main: _checkGeneratedBindingsPubspecVersion in pigeon_lib_internal.dart (line 2779 at 3999bc2) still uses !dartOutPath.startsWith(basePath). With basePath: 'app' and dartOut: 'app_bindings/lib/x.dart', it looks for the bindings file at app_bindings/... instead of app/app_bindings/..., finds nothing and skips the version check without saying so. It probably wants isWithinOrEquals like the other sites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants