[analyzer] Normalize inherited getter and setter types before comparing - #63945
[analyzer] Normalize inherited getter and setter types before comparing#63945Yusufihsangorgel wants to merge 2 commits into
Conversation
The check compared the combined getter and setter types with `==`, which is structural. The notion of "same type" used elsewhere in the language normalizes `FutureOr`, so `NORM(FutureOr<Object>)` is `Object` and a getter typed `Object` agrees with a setter typed `FutureOr<Object>`. `dynamic` and `Object?` normalize to themselves and keep reporting. Bug: dart-lang#63635 TEST=pkg/analyzer/test/src/diagnostics/different_inherited_getter_and_setter_types_test.dart
|
Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at: https://dart-review.googlesource.com/c/sdk/+/530180 Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR). |
The spec says the field takes the parameter type of the combined setter signature (resources/type-system/inference.md:249), and the comment above this code already said so, but the assignment took the getter type. Both sides are the same type, so this only changes which representative the element model keeps: `FutureOr<Object>` rather than `Object`, and the setter's type alias rather than the getter's. The normalized comparison moves into TypeSystemImpl as areStructurallyEqualAfterNormalization.
|
https://dart-review.googlesource.com/c/sdk/+/530180 has been updated with the latest commits from this pull request. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
https://dart-review.googlesource.com/c/sdk/+/530180 has been updated with the latest commits from this pull request. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
Gerrit CL has been approved, please wait for a reviewer to merge it. There are also new comments on the CL, please review them and respond if necessary because reviewer might have requested clarifications or possibly some final changes and will not merge the CL until their questions have been answered. |
|
Gerrit CL has been approved, please wait for a reviewer to merge it. There are also new comments on the CL, please review them and respond if necessary because reviewer might have requested clarifications or possibly some final changes and will not merge the CL until their questions have been answered. |
|
Hi @Yusufihsangorgel, thanks for contributing a substantial amount of work! However, as I wrote here, we must approach this topic very cautiously because it changes existing behavior at a rather fundamental level of the language. The language team will have to clarify the situation first, and we might need to refine and clarify the rules in this area. So I'm afraid this PR should not be landed at this time. I hope you don't think we're wasting your time by putting your work on hold like that, but it wouldn't be good if we make changes to the implementation in this area, and it turns out to be slightly different from what we actually want in the end. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
CL has new comments, please view and respond to them in Gerrit. If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews. Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send. |
|
Not at all, and I've withdrawn the front-end offer over on the issue. The branch stays where it is. |
The check compared the combined getter and setter types with
==, whichis structural. The notion of "same type" used elsewhere in the language
normalizes
FutureOr, soNORM(FutureOr<Object>)isObjectand agetter typed
Objectagrees with a setter typedFutureOr<Object>.dynamicandObject?normalize to themselves and keep reporting.This is the analyzer half of what
@eernstgdescribed in the issue, onthe example
@scheglovasked about there.Two tests.
ObjectagainstFutureOr<Object>was red before this changeand is green after;
dynamicagainstObject?pins the behavior thatmust not move.
The CFE half is still open.
inferFieldTypeinmembers_node.dartcompares the same two types with kernel
==, so it reports on the exampletoday and the two disagree until it changes as well. Happy to follow it
into the front end if that is the order you want.
Bug: #63635
TEST=pkg/analyzer/test/src/diagnostics/different_inherited_getter_and_setter_types_test.dart
Contribution guidelines:
dart format.Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.