Skip to content

Update the docs for @reopen to finalized spec #51551

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
pq opened this issue Feb 27, 2023 · 5 comments
Closed

Update the docs for @reopen to finalized spec #51551

pq opened this issue Feb 27, 2023 · 5 comments
Labels
devexp-pkg-meta Issues related to package:meta legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@pq
Copy link
Member

pq commented Feb 27, 2023

The spec has changed a bit since we drafted the initial docs for the @reopen annotation, making them no-longer valid.

Here's what we currently have:

Annotation for intentionally loosening restrictions on subtyping.

Indicates that the annotated class or mixin declaration
intentionally allows subclasses to implement or extend it, even
though it has a superclass which does not allow that.

A declaration annotated with `@reopen` will not generate warnings from the
`implicit_reopen` lint. That lint will otherwise warn when a subclass *C*
removes some of the restrictions that a superclass has.

* A class or mixin prevents inheritance if it's marked interface, or if it
  is marked sealed and it extends or mixes in another class which prevents
  inheritance.
* We give a warning if a subclass extends or mixes in another class which
  prevents inheritance, and the subclass is marked base, or is not marked
  `final`, `interface` or `sealed`.
* A class or mixin requires inheritance if it's marked `base`, or if it is
  marked `sealed` and it extends or mixes in another class or mixin which
  requires inheritance.
* We give a warning if a subclass extends or mixes in another class which
  requires inheritance, and the subclass has no modifier or is marked
  `interface`.
* A class or mixin prevents subclassing if it's marked `final`, or if it is
  marked `sealed` and it extends, mixes in, or implements the interface of
  another class or mixin which prevents subclassing.
* We give a warning if a subclass or sub-mixin extends, mixes in, implements
  the interface of, or has as an on type a class or mixin which prevents
  subclassing, and the subclass or sub-mixin has no modifier or is marked
  `interface` or `base`.

In addition, tools, such as the analyzer, can provide feedback if

* The annotation is applied to anything other than a class or mixin.
* The annotation is applied to a class or mixin which does not require it.
  (The intent to reopen was not satisfied.)

See also #58976

/fyi @kallentu @lrhn @munificent

@pq pq added P1 A high priority bug; for example, a single project is unusable or has many test failures legacy-area-analyzer Use area-devexp instead. devexp-pkg-meta Issues related to package:meta labels Feb 27, 2023
@pq
Copy link
Member Author

pq commented Feb 27, 2023

Some background for recent changes in dart-lang/language#2755.

See also: dart-lang/language#2871

@pq
Copy link
Member Author

pq commented Mar 2, 2023

/fyi @eernstg in case you're up for taking a crack at some clarification here

@eernstg
Copy link
Member

eernstg commented Mar 3, 2023

Here's one possible direction for the evolution of this text:

Annotation for intentionally loosening restrictions on subtyping.

Indicates that the annotated class, mixin, or mixin class declaration
intentionally allows subtypes to implement it, or extend it, or mix it
in, even though it has some superinterfaces whose constraints are
thereby indirectly violated. For example, if the superinterface is
`interface`, and the annotated declaration is a plain subclass, we can
indirectly inherit methods from the superinterface, even though that's
an error if it is done directly.

A declaration annotated with `@reopen` will not generate warnings from the
`implicit_reopen` lint. That lint will otherwise warn when a subtype *C*
has restrictions that are not sufficient to enforce the restrictions
declared by class modifiers on one or more superinterfaces.

First, some terminology. A class, mixin, or mixin class declaration
_prevents inheritance_ if it is marked `interface` or `final`, or it is marked
`sealed`, and an `interface` or `final` modifier is implicitly present, based
on the modifiers of its superinterfaces, or it is an anonymous mixin
application, and it is again implicitly `interface` or `final`. Finally, we can
specify the warning:

We give a warning if a declaration extends or mixes in another declaration
which prevents inheritance, and the subtype is not marked `final`,
`interface` or `sealed`.

Issues:

  • We can't say "the subtype removes restrictions", because that's perfectly ok in some situations: If a superinterface S is interface (so we can't inherit member implementations) and the subtype C uses implements S, then we can remove the restriction (because we can allow inheritance of member implementations from C without hurting S). So the property of interest is that we will indirectly allow the constraints on S to be violated, not that C has a different set of constraints.
  • I've tried to weasel around the possible forms of declarations. We could say 'class, mixin, or mixin class' again and again. I've chosen to just say 'declaration' in a few places, in order to avoid all those words.
  • The warning can also be a compile-time error (when it's cross-import), but I did not mention this. My assumption is that the reader won't care when they get a warning that there is a similar situation where they would have gotten an error, or vice versa.
  • The second bullet was removed (this is now an error, even when it is in the same library).
  • I deleted the definition of 'prevents subclassing'; it is now an error (even in the same library) to have a declaration D with a direct superinterface declaration D2, if D2 is base or final (including implicit modifiers of sealed declarations and mixin applications), and D is not base, not final, and not sealed. So we don't need the concept of 'prevents subclassing' here, because those situations are not linted.
  • It gets heavy if we specify in detail how the implicitly induced class modifiers of mixin applications and of sealed declarations are computed. I'd hope that developers can simply hover on sealed or on the mixin name in order to see those modifiers.

@pq
Copy link
Member Author

pq commented Mar 3, 2023

This is just fantastic. Thank you Erik!

@pq
Copy link
Member Author

pq commented Mar 7, 2023

Fixed w/ 4c7e878

@pq pq closed this as completed Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-pkg-meta Issues related to package:meta legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

2 participants