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

Implement @sealed-based analyzer codes #34232

Closed
srawlins opened this Issue Aug 22, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@srawlins
Copy link
Member

commented Aug 22, 2018

I think we need 3 codes for analyzer's @sealed analysis:

  • INVALID_SEALED_ANNOTATION, which reports when @sealed annotates something other than a class or mixin.
  • INVALID_USE_OF_SEALED_CLASS, which reports when a @sealed class is used as a super-class, outside of its package.
  • UNSEALED_SUB_CLASS_OF_SEALED_CLASS, which reports when a valid (same-package) sub-class of a sealed class is not also annotated with @sealed. deciding against; instead dart-lang/dartdoc#1886 can help to document.

dart-bot pushed a commit that referenced this issue Aug 23, 2018

Implement INVALID_SEALED_ANNOTATION analyzer Hint
This follows the pattern of other meta-annotations, when it comes to the AST:

* Element gains a new `hasSealed` getter. Many overrides.
* ElementAnnotation gains a new `isSealed` getter.

Bug: #34232
Change-Id: If8ae8e16b500125cb3b92b3cf83d46de6ca6ee23
Reviewed-on: https://dart-review.googlesource.com/71227
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

dart-bot pushed a commit that referenced this issue Dec 13, 2018

Add WorkspacePackage and various implementations, for determining whe…
…ther two files are in the "same package."

This includes _BasicWorkspacePackage for _BasicWorkspace, BazelWorkspacePackage
for BazelWOrkspace, GnWorkspacePackage for GnWorkspace, and
PackageBuildWorkspacePackage for PackageBuildWorkspace.

Bug: #34232
Change-Id: I686b529f460a108b8477d109d07fb29563dd7314
Reviewed-on: https://dart-review.googlesource.com/c/81523
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

dart-bot pushed a commit that referenced this issue Jan 2, 2019

Enforce sealed annotation on classes and mixins
This CL replaces https://dart-review.googlesource.com/c/sdk/+/75941
(there were some large refactorings, and I got a new laptop and lost my Git repo)

Bug: #34232
Change-Id: Ia0746b94a23ccb757bc209e76098243896be8c55
Reviewed-on: https://dart-review.googlesource.com/c/88127
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Hi @bwilkerson, what do you think about the 3rd bullet above? I'm thinking we can do without, as other rules that could have a similar requirement, don't.

E.g. all subclasses of an @immutable class are not required to also be labelled @immutable. Instead the rules of @immutable are enforced on all subclasses of an @immutable class, instead of direct subclasses. The same is currently done for @sealed, so I think it would be nice to not require every single subclass of a @sealed class to be explicitly marked as such.

WDYT?

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I think it's perfectly reasonable for an annotation to impact declarations other than the one it's textually associated with. We have several like that (such as immutable). And I think it makes sense for sealed because I think the author's expectation is that there won't be any subtypes, direct or indirect.

But I also think there's an interesting documentation question. If a class is sealed indirectly, then it's harder for a user to see that it's sealed. I have to wonder whether dartdoc documents these transitively applied annotations. (@jcollins-g)

But I'd define sealed as not being required on subclasses and resolve the documentation question more globally.

@jcollins-g

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

If the analyzer tells us an annotation exists in the element metadata, we should document it, but we don't attempt to transitively apply annotations outside of what the analyzer tells us. It's possible we could implement that though. Dartdoc already knows about a handful of "special" objects that are documented in an unusual fashion (Object, Interceptor, and @pragma) and this would be another two added to that list. While this adds special casing, it'd be relatively straightforward to do nice things like refer directly to the class declaring @sealed in documentation of indirectly sealed classes.

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

OK, I've opened dart-lang/dartdoc#1886. Let's call this complete then 😄.

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

The only thing left to do is release meta 1.1.7. @bwilkerson is that something you can do?

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Yes. Is everything ready?

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Oops, lemme correct the doc comment on sealed before releasing. https://dart-review.googlesource.com/c/sdk/+/88289

dart-bot pushed a commit that referenced this issue Jan 3, 2019

Correct doc comment for sealed
As per discussion at #34232

Bug: #34232
Change-Id: Ib0a7207a43167b5e69973fc3abffa197e0bdafe5
Reviewed-on: https://dart-review.googlesource.com/c/88289
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

OK meta package should be ready to release!

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

done

@srawlins

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

I'm calling this one closed. Thanks for everyone's input!

@srawlins srawlins closed this Jan 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.