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

Rules of @reopen annotation and lint #2730

Closed
lrhn opened this issue Dec 18, 2022 · 4 comments
Closed

Rules of @reopen annotation and lint #2730

lrhn opened this issue Dec 18, 2022 · 4 comments
Assignees
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.

Comments

@lrhn
Copy link
Member

lrhn commented Dec 18, 2022

The feature specifiction states, as a suggested lint/annotation (and the suggestion may be followed, q.v. dart-lang/linter#3920):

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 class or mixin marked interface or final and is not itself marked interface or final.
  • Extends, implements, or mixes in a class or mixin marked base or final and is not itself marked base, final, or sealed.

The rationale for reopen and @reopen in the "Trust but verify" section seems to be that you should explicitly say when you grant a capability that was removed in a superclass, other than by sealed. Or maybe it only applied if you didn't write any other modifier, it's not entirely clear.

The rules here do not match that, since they do not allow a final class's subclass to be marked as sealed, but they do allow a final subtype (through implements) to have a base subclass. That's removing a restriction, but not leaking the original implementation.

There is (probably) a reason for this choice, but it's not made explicit in the proposal. I think it is:

  • Private implementation: If a type cannot be publicly subclassed in a way which inherits implementation, a library-local subtype which does inherit the implementation must also not allow that implementation to be publicly inherited.
    (Still should allow a sealed subclass, so add ", or sealed" to the first item.)

  • Private interface: If a type's interface cannot be publicly implemented, a library local subtype must also not be able to have its interface publicly inherited.

If something is final, it must satisfy both and again be final or sealed.

This still doesn't prevent leaking things through a sealed class declaration, because being sealed does not impose restrictions on direct subclasses. Would we want to check transitively through superclasses, past sealed classes, to see if we are leaking something further up the supertype tree?

(If we don't give the rationale behind the rules, it's impossible to see whether the rules/lint is working as intended.)

Another worry is that a sealed subtype can subtype any class, and it can have any subclasses without warning, so there is no transitive checking.
If I have final class Foo, sealed class Bar extends Foo, and class Baz extends Bar, then there is no requirement for an @open annotation on Baz, even though it exposes the final class Foo implementation to other libraries.

The issue is that sealed is not intended as inherited, the other restrictions are, and the rules here try to enforce the transitive restrictions through one-step rules. That chain of restriction breaks at a sealed class.

So maybe say when a class declaration C needs an @open annotation by defining a function:

restrictions(class or mixin declaration) ↦ {final,sealed,base,none}

restrictions(C) ::=
* If C does not have a sealed modifier, the result is the final, interface, or base modifier of the declaration, or none if there are no such modifier.
* Otherwise C is sealed and has no other modifier.
* Then the result is inheritedRestrictions(C).

inheritedRestrictions(class or mixin declaration) ↦ {final,sealed,base,none}

inheritedRestrictions(C) ::=

  • Let m be none.
  • If C is a mixin declaration, let I1..In be the declarations of the interfaces of its implements clause (if any) and let S1..Sk be the declarations of the interfaces of the required super-types of its on clause (if any).
  • If C is a class declaration, let I1..In be the declarations of the interfaces of its implements clause (if any), let S1 be the declaration of the superclass type of the extends clause (if any), and let S2..Sk be the declarations of the mixins of the with clause (if any).
  • For each S in S1..Sk:
    • If S is not declared in the same library as C, continue to the next S.
    • Let mS be restrictions(S, lib).
    • Let m be MUP(m, mS).
  • For each I in I1..In.
    • If I is not declared in the same library as C, continue to the next I.
    • Let mI be restrictions(I, lib).
    • If mI is base or final (cannot be implemented), let m be MUP(m, mI).
  • The result is then m.

Define MUP as a least upper bound on the lattice:

         final
        /     \
     base    interface
        \     /
         *none*

We then say that a class or mixin declaration D needs an @open annotation if the declaration does not have a sealed (or final) modifier, so it's modifier m is one of base, interface or none, and MUP(m, inheritedRestrictions(D)) ≠ m.
(A final declaration cannot fail the requirement, so we can short-circuit the test.)

@lrhn lrhn added the class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins. label Dec 18, 2022
@munificent
Copy link
Member

Do you still want this assigned to me? You and @kallentu are definitely the resident experts on class modifiers now. :)

@lrhn
Copy link
Member Author

lrhn commented Apr 5, 2023

Nope, let's assign to myself instead. On the off chance I'll remember anything after Easter.

It might also not be necessary to specify the lint precisely in the feature specification. It's already implemented. (But let's check that it does what we want it to.)

@lrhn lrhn assigned lrhn and unassigned munificent Apr 5, 2023
@leafpetersen
Copy link
Member

Anything left to do here, or can this be closed out?

@lrhn
Copy link
Member Author

lrhn commented May 10, 2023

I believe the linter people are well on top of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.
Projects
None yet
Development

No branches or pull requests

3 participants