Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Account for records in type relatedness checks #4266

Merged
merged 4 commits into from
Apr 10, 2023
Merged

Conversation

srawlins
Copy link
Contributor

@srawlins srawlins commented Apr 8, 2023

Fixes dart-lang/sdk#59108

Uses isAssignableTo to handle relatedness, accounting for shape and subtypes.

@coveralls
Copy link

coveralls commented Apr 8, 2023

Coverage Status

Coverage: 96.43% (+0.01%) from 96.416% when pulling e4ee352 on unrelated-records into fffb74c on main.

Copy link
Contributor

@pq pq left a comment

Choose a reason for hiding this comment

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

Super awesome!

Might be good to have @bwilkerson take a quick look but LGTM!

@srawlins srawlins requested a review from bwilkerson April 10, 2023 14:52
@@ -282,6 +261,39 @@ class InterfaceTypeDefinition {
}
}

extension on TypeSystem {
bool interfaceTypesAreUnrelated(InterfaceType type1, InterfaceType type2) {
// In this case, [leftElement] and [rightElement] each represent
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming the parameters to leftElement and rightElement. I think they read better. Otherwise, consider updating the comments to match the current names.

(And I don't understand what "this case" is referring to here, but that's not your problem because you didn't write the original code.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good note; renamed.

I also really didn't understand what that comment referred to. After looking for a few minutes, I decided to just remove it.

@@ -18,6 +18,28 @@ class UnrelatedTypeEqualityChecksTestLanguage300 extends LintRuleTest
@override
String get lintRule => 'unrelated_type_equality_checks';

test_recordAndInterfaceType_unrelated() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have some tests involving records with named fields, and maybe some that have both positional and named fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@srawlins srawlins merged commit eb0ca50 into main Apr 10, 2023
@srawlins srawlins deleted the unrelated-records branch April 10, 2023 17:01
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

"unrelated type" checking does not account for record types
4 participants