Skip to content

Add a hint @nonVirtual for class members #34378

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
matanlurey opened this issue Sep 5, 2018 · 9 comments
Closed

Add a hint @nonVirtual for class members #34378

matanlurey opened this issue Sep 5, 2018 · 9 comments
Labels
customer-google3 devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Sep 5, 2018

... or, to help developers when defining the intended use of a class, and to help users use a class properly (i.e. as intended by the class). I'd find this extremely useful when working on fairly complex classes with lots and lots of member fields and methods.

Similar to the spirit of @sealed (#27372):

import 'package:meta/meta.dart';

class X {
  @nonVirtual
  void alwaysPrintFoo() {
    print('Foo');
  }
}

class Y extends B {
  // HINT: X.alwaysPrintFoo is marked @nonVirtual and should not be overridden.
  void alwaysPrintFoo() {
    print('Bar');
  }
}

General concept:

  • Issue a hint when a member annotated with @nonVirtual is overriden.

Because of how this annotation would need to work, I imagine it would also need to:

  • Issue a hint when a class with one or more @nonVirtual members is implemented (implements - not extends/with), as by definition a class with @nonVirtual member requires the original implementation:

    import 'package:meta/meta.dart';
    
    class X {
      @nonVirtual
      void alwaysPrintFoo() {
        print('Foo');
      }
    }
    
    // HINT: Cannot implement a class with one or more `@nonVirtual` members.
    abstract class Y implements X {}
  • Issue a hint when an abstract member is annotated with @nonVirtual. Also impossible:

    import 'package:meta/meta.dart';
    
    abstract class X {
      // HINT: Cannot define a `@nonVirtual` abstract member.
      @nonVirtual
      void alwaysPrintFoo();
    }

/cc @davidmorgan @srawlins for thoughts.

@matanlurey matanlurey added legacy-area-analyzer Use area-devexp instead. devexp-warning Issues with the analyzer's Warning codes customer-google3 labels Sep 5, 2018
@matanlurey matanlurey changed the title Add @nonVirtual for class members Add a hint @nonVirtual for class members Sep 5, 2018
@rkj
Copy link

rkj commented Sep 5, 2018

I do like it, would be definitely helpful in many APIs. Why not call it @final to align with Java?

@matanlurey
Copy link
Contributor Author

I believe that is a reserved word/keyword in Dart. It could be @finalMember, but meh.

(I don't care about the name, to be clear, it can be whatever folks agree on)

@matanlurey
Copy link
Contributor Author

An alternative is just to support @sealed on member methods/fields, of course!

/cc @srawlins

@bwilkerson bwilkerson added the type-enhancement A request for a change that isn't a bug label Sep 16, 2018
@filiph
Copy link
Contributor

filiph commented Apr 14, 2019

I keep getting into situations where I'd like to have this functionality.

For reference, an old school issue from @danrubel asking for language-level support of this is here: #3928. I'll update that issue to link here. I do think it's better (and more practical) to implement this via pkg:meta rather than having it in the language.

@rayfarias56
Copy link

Also just ran into a situation where this would be useful. Any updates on the priority? I'm willing to contribute the change, just not sure where to begin.

@hacker1024
Copy link
Contributor

It's now October 2019, and this issue's been open for over a year. can someone on the Dart team let us know what they think about this?

@pq
Copy link
Member

pq commented Oct 23, 2019

I'm all for revisiting this. Specifically, it could potentially be used to disallow overriding Widget equality in Flutter (which otherwise we'll have to analyze specifically for). See #58035 and it's inspiration: flutter/flutter#41220 (comment).

@pq
Copy link
Member

pq commented Oct 23, 2019

OK, running w/ @nonVirtual:

https://dart-review.googlesource.com/c/sdk/+/122701

dart-bot pushed a commit that referenced this issue Oct 23, 2019
See: #34378

Change-Id: Ie61b8bddaef01cdbf47da9a8ca6915cbd86b21a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/122701
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
dart-bot pushed a commit that referenced this issue Oct 24, 2019
see: #34378

Change-Id: I0fe67c56bf7c08f211b6bf89d84c5495e32601a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/122742
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@pq
Copy link
Member

pq commented Oct 30, 2019

Implemented in server and landed in package:meta:

image

Looking forward to seeing how folks take advantage!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-google3 devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants