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

@override docs clarification #43622

Closed
feinstein opened this issue Sep 30, 2020 · 5 comments
Closed

@override docs clarification #43622

feinstein opened this issue Sep 30, 2020 · 5 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-documentation A request to add or improve documentation

Comments

@feinstein
Copy link

The @override docs say Use the @override annotation judiciously, but there's no reason why. Does it have any compiler performance impacts?

@bwilkerson
Copy link
Member

No, the annotation doesn't impact performance.

The purpose of the annotation is to express intent to the static analysis tooling. In this case it lets the tool know that you defined the method in order to override (or implement) behavior from a supertype. If the supertype changes such that the method is no longer overriding the method it once was (for example, if the method was removed or renamed) then the tooling can let you know that your implementation might need to be updated in response to that change. Having the annotation helps the tooling find potential errors statically.

@feinstein
Copy link
Author

Yes, but why does the docs say we should use it judiciously?

@bwilkerson
Copy link
Member

That I don't know. In the analyzer package, and several other related packages, we use it liberally and have never seen any downside as a result of doing so.

We even go so far as to use the annotate_overrides lint (which is part of the pedantic rule set) to ensure that we don't miss adding the annotation anywhere where we could. That lint also lets us know if a supertype adds a method that happens to collide with one we'd already defined, which is also useful information for catching bugs.

@mit-mit
Copy link
Member

mit-mit commented Oct 1, 2020

cc @munificent

@lrhn
Copy link
Member

lrhn commented Oct 1, 2020

The override annotation, and its documentation, was introduced before package:meta existed, and there was no general consensus about whether you should use the annotation or not. That was in 2013. There still isn't consensus, but it's definitely tipping in one direction.

If we added the annotation today, the annotation would just have been added to package:meta. We considered moving it there for Dart 2.0, but decided that it would be too breaking, for too little benefit, but you should think of it as any other annotation from package:meta.

The documentation was written by someone who didn't feel strongly that it should be used everywhere it could (because it adds a lot of syntactic overhead and clutter for little perceived benefit), but you should only use it to detect breakage across boundaries where you don't control both ends. That's what the documentation states.

Use it if you judge that it brings you a benefit, and don't if you don't want to. The language, and the language style guide, does not recommend either direction, it's entirely up to you.

People have generally chosen to use it everywhere, so the documentation is probably a little outdated compared to actual current usage.

@mit-mit mit-mit added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Oct 2, 2020
@lrhn lrhn added library-core type-documentation A request to add or improve documentation labels Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-documentation A request to add or improve documentation
Projects
None yet
Development

No branches or pull requests

4 participants