Repository navigation
fix(mix_generator): compare generic target tear-offs structurally - #1105
kauandotnet wants to merge 1 commit into
Conversation
validateGenericTargetTearOff compared the tear-off type's type parameters with the class's by element identity. Under build_runner, analyzer 11 and later can return the constant's function type with freshly created type parameters that have no enclosing element, so a direct RemixCheckboxGroupItem.new target was rejected as instantiated once other libraries had been resolved in the same build (a filtered build of the one file passed). Check the shape instead: one type parameter per class type parameter, with the class parameter's name and bound, declared by the class or by no element (never by a typedef), and a return type that instantiates the class with exactly those parameters. Instantiated and typedef tear-offs are still rejected.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
leoafarias
left a comment
leoafarias
left a comment
There was a problem hiding this comment.
I looked at the fix and the existing mix_generator validation suite — the structural compare is the right direction for the analyzer 11+ enclosingElement == null case, but I’m holding merge until we have tests for it.
Please add coverage in packages/mix_generator/test/integration/generator_validation_test.dart (same style as the other @MixableSpec(target:) cases): a direct generic tear-off that must pass, plus rejects for typedef / instantiated / nullable type-arg shapes. Then run the mix_generator tests and paste the result here.
Stargate already depends on this via a path override, so I’d rather not ship without a regression net.
| declaredBy.library.uri != owner.library.uri)) || | ||
| parameter.bound?.getDisplayString() != | ||
| classParameter.bound?.getDisplayString()) { | ||
| return false; |
There was a problem hiding this comment.
Need a unit/integration test that hits this path when the tear-off type params have enclosingElement == null (the analyzer 11+ / build_runner case that broke Stargate). The single-file filtered build isn’t enough — that’s why the bug slipped through.
Also please reject nullable type args here (C<T?>). Right now we only check TypeParameterType + name, so a nullable argument can still look “structural” and get accepted. Same contract as “direct, uninstantiated”.
There was a problem hiding this comment.
Ignore the nullable type-arg note above — that one’s on me. Only ask here is the regression test for the enclosingElement == null path, then run the mix_generator suite and paste the result.
|
Quick clarification on my review: the only ask is adding the regression tests (especially the analyzer 11+ |
leoafarias
left a comment
leoafarias
left a comment
There was a problem hiding this comment.
A few more concrete asks before I’d merge — same hold as before, just spelling out the test shape and two code gaps.
| returnType is! InterfaceType || | ||
| returnType.typeArguments.length != own.length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Blocker: please unit-test this path with a FunctionType whose type params have enclosingElement == null (orphan params), not a full multi-library build_runner run. That’s the analyzer 11+ case that bit Stargate — if we only cover it via an integration build, it can regress quietly.
| parameter.bound?.getDisplayString() != | ||
| classParameter.bound?.getDisplayString()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Blocker: reject nullable type args here (C<T?>). Right now we only check TypeParameterType + name, so a nullable argument can still look structural and get accepted. Same “direct, uninstantiated” contract as the rest of this guard.
| if (parameter.name != classParameter.name || | ||
| (declaredBy != null && | ||
| (declaredBy is! InterfaceElement || | ||
| declaredBy.name != owner.name || |
There was a problem hiding this comment.
Medium: comparing bounds with getDisplayString() is a bit fragile — equivalent bounds can disagree as text. Prefer the analyzer type system (or an element-level bound compare) so we don’t false-reject. Fine as a follow-up if you want the Stargate unblock first, but I’d rather do it in this PR.
validateGenericTargetTearOffcompared the tear-off's type parameters with the class's by element identity. Under build_runner, analyzer 11 and later can return the constant's function type with freshly created type parameters that have no enclosing element. As a result, a directRemixCheckboxGroupItem.newtarget was rejected as instantiated once other libraries had been resolved in the same build. A filtered build of the one file passed.The check now compares shape instead:
Instantiated and typedef tear-offs are still rejected.
Found in: Stargate Studio's build on analyzer 13, where 2.2.1 rejects its checkbox group. Stargate uses this commit through a temporary path override until a release carries the fix.
Testing: this PR adds no regression test. The failure only appears in a multi-library build_runner run.