Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep operator== parameter type as Object #1017

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

srawlins
Copy link
Contributor

@srawlins srawlins commented Dec 7, 2023

Fixes #1016

@rrousselGit
Copy link
Owner

I appreciate the PR, but I don't fully understand the problem.

What are we really solving here? Performance?

@srawlins
Copy link
Contributor Author

srawlins commented Dec 8, 2023

No, there shouldn't be any performance implications.

These operator == overrides explicitly expand the parameter type from Object to dynamic. Why?

No Dart runtime passes a null value to an override == implementation. Explicitly expanding the parameter type does not seem to serve a purpose, and can lead to confusion that the function will ever see a null argument value.

When someone knows that Object.== takes an Object, and that one of these classes has expanded the parameter type to be dynamic, they may question why, and look for the important reason that the type has been changed. But it's a red herring; there is no reason. If one of these implementations did something with a null argument value, like:

bool operator ==(dynamic other) {
  if (other == null) print('boo');
  return ...
}

it would be surprising that the code never prints 'boo'. The only effect that expanding the parameter type to dynamic seems to have is confusion.

@rrousselGit
Copy link
Owner

I was thinking that this could be useful in cases someone wants to override == by hand.
But it looks like from the interface POV, == isn't overridden, so it doesn't block them.

So this looks to be purely syntactic. Which I don't think adds a lot of value for generated code.

I'll merge it anyway, but if what triggered this PR is a lint rule, chances are that lint rule shouldn't trigger on generated files.
Users generally shouldn't read its source. Stylistic choices there shouldn't matter.

@rrousselGit rrousselGit merged commit 8799cec into rrousselGit:master Dec 8, 2023
7 checks passed
@srawlins srawlins deleted the operator-eqeq branch December 8, 2023 14:15
@srawlins
Copy link
Contributor Author

srawlins commented Dec 8, 2023

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated freezed classes expand == operator parameter to be nullable (dynamic)
2 participants