Skip to content

implement @reopen #50977

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

Closed
Tracked by #50746
pq opened this issue Jan 10, 2023 · 7 comments
Closed
Tracked by #50746

implement @reopen #50977

pq opened this issue Jan 10, 2023 · 7 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. devexp-pkg-meta Issues related to package:meta P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Milestone

Comments

@pq
Copy link
Member

pq commented Jan 10, 2023

From the base-interface-final spec:

@reopen lint
...

A metadata annotation @reopen is added to package meta and a lint "require_reopen" is added to the linter. When the lint is enabled, a lint warning is reported if a class or mixin is not annotated @reopen and it:

  • extends or mixes in a type marked interface or final and is not itself marked interface or final.
  • extends, implements, or mixes in a type marked base or final and is not itself marked base, final, or sealed.

See also: #58976

@pq pq added devexp-pkg-meta Issues related to package:meta P2 A bug or feature request we're likely to work on labels Jan 10, 2023
@pq pq added this to the Dart 3 beta 1 milestone Jan 10, 2023
@pq pq added area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. type-enhancement A request for a change that isn't a bug labels Jan 10, 2023
@bwilkerson
Copy link
Member

I don't understand the value of this annotation. The proposed lint ensures that the annotation is provided, but other than silencing the lint the annotation appears to have no impact on analysis. If the only value of the annotation is to silence a lint, and the only value of the lint is to require the annotation, then neither has any value. What am I missing?

@munificent

@pq
Copy link
Member Author

pq commented Jan 11, 2023

The way I understand it, require_reopen will likely be a core or recommended lint which means that in the common case folks will get a diagnostic if they try and un-seal a sealed class without explicitly marking it with @reopen. If I understand the proposed value, it's to ensure that sealed classes aren't opened accidentally.

Or maybe I'm missing what you're missing? 😉

@lrhn
Copy link
Member

lrhn commented Jan 11, 2023

What @pq says. The lint is to avoid reopening a closed class accidentally in a subclass. The @reopen annotation silences the lint and states that the reopening is deliberate, the same way the unawaited future silences the unawaited_futures lint when not awaiting is intentional.

It's a lint with false negatives, but still with (expected) enough value that it's worth dealing with those negatives.

@bwilkerson
Copy link
Member

Ok, thanks to the analogy I think I understand. In the same way that not-awaiting a future is a foot gun, so is not sealing a subclass of a sealed class (etc. for the other conditions).

Then the purpose of the lint isn't to require a @reopen annotation, it's to prevent unintentionally reopening a class or mixin. I'd like to propose, then that we name the lint something like unintentional_reopening so that the purpose of the lint is more clear.

@lrhn
Copy link
Member

lrhn commented Jan 13, 2023

SGTM. @munificent

@munificent
Copy link
Member

Ok, thanks to the analogy I think I understand. In the same way that not-awaiting a future is a foot gun, so is not sealing a subclass of a sealed class (etc. for the other conditions).

Then the purpose of the lint isn't to require a @reopen annotation, it's to prevent unintentionally reopening a class or mixin.

Exactly right. Though, personally, I'm not yet convinced that this lint should be recommended or core. We'll see how much of a footgun it actually is in practice.

copybara-service bot pushed a commit that referenced this issue Jan 23, 2023
See: #50977

Change-Id: I8bc6547a3f5e2cf4bb8913b1d6dfb8a618fc2022
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278888
Reviewed-by: Kallen Tu <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@pq
Copy link
Member Author

pq commented Jan 24, 2023

Implemented in c8df557

@pq pq closed this as completed Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. devexp-pkg-meta Issues related to package:meta P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants