Skip to content

[lint] meta attribute for to warn/error on non-const usage #40269

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
dnfield opened this issue Jan 22, 2020 · 8 comments
Closed

[lint] meta attribute for to warn/error on non-const usage #40269

dnfield opened this issue Jan 22, 2020 · 8 comments
Labels
devexp-pkg-meta Issues related to package:meta legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@dnfield
Copy link
Contributor

dnfield commented Jan 22, 2020

Flutter has a class called IconData.

It generally does not make sense to use a non-const instance of IconData. It's meant to refer to a glyph ID in a font asset file, and the font asset file is full of icons, not alphabetical characters.

We're also building out tooling to help find constant instances of IconData in the compiled kernel file, and then shake out the font glyphs that aren't used to save binary space. However, this won't work if a user has a non-const instance, e.g. new IconData(123) instead of const. We can detect that and fail, but it would be super nice to have a lint where we could do something like:

@mustBeConst
@immutable
class IconData {
  const IconData(...);
  ...
}

Which would generate some kind of linting warning if I did something silly like

var iconBad = Icon(icon: IconData(1234)); // lint
const iconGood = Icon(icon: IconData(1234)); // no lint
@bwilkerson
Copy link
Member

We have something close to what you're asking for, but it allows an exception that you might not want. It's in the meta package:

/// Used to annotate a const constructor `c`. Indicates that any invocation of
/// the constructor must use the keyword `const` unless one or more of the
/// arguments to the constructor is not a compile-time constant.
///
/// Tools, such as the analyzer, can provide feedback if
///
/// * the annotation is associated with anything other than a const constructor,
///   or
/// * an invocation of a constructor that has this annotation is not invoked
///   using the `const` keyword unless one or more of the arguments to the
///   constructor is not a compile-time constant.
const _Literal literal = _Literal();

@dnfield
Copy link
Contributor Author

dnfield commented Jan 22, 2020

That's super close to what I want, and I think it'd work if I could use it like

@immutable
class IconData {
  @literal
  const IconData(@literal this.codePoint, {@literal this.fontFamily, @literal this.fontPackage, @literal this.matchesTextDirection});

To show that all the params (Strings, ints, and bools) have to be literals too.

@bwilkerson
Copy link
Member

I think I'd rather have a dedicated annotation (like mustBeConst) so that developers can't forget to mark some of the parameters. (And it would keep the code less cluttered.)

@srawlins srawlins added area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. pkg-meta type-enhancement A request for a change that isn't a bug labels Jan 22, 2020
@a14n
Copy link
Contributor

a14n commented Jan 23, 2020

Isn't prefer_const_constructors enough? Should we enable it by default in the default pubspec.yaml?

@dnfield
Copy link
Contributor Author

dnfield commented Jan 23, 2020

This is a little different than prefer_const_constructors. That just says if you can, use it.

I want something that tells the user they're doing something wrong if they're not const in this case.

@jellynoone
Copy link

jellynoone commented Apr 20, 2022

I would also like to see something @dnfield recommended, but from the perspective of sql queries or similar where the parameter given should really be a known object i.e. it shouldn't be possible to pass a dynamic value since you should be able to guarantee all the possible routes.

For instance:

class Db {
  execute(@literal String sql, [Map<String, dynamic> parameters]) {}
}

where the sql is know to be a literal query like Select * from my_table or is a string concatenated from literals i.e.

const myTable = 'my_table';

void main() {
  Db().execute('Select * from $my_table');
}

Additionally, it would be great if functions could also produce literal results, for instance if you had:

String join(@literal str1, @literal str2) => '$str1$str2';

Where returned result from join would return a String considered to be literal.

Although, I suppose this could also be considered as tracking whether a function returns a constant result. Would it then be possible to also use some functions in constant expressions?

Meaning if it is known at compile time that the top-level function only uses constants or no outside scope and that all of it's parameters are literals then it would be guaranteed to always return the same result.

So the bellow could also become valid?

const aAndB = join('A', 'B');

Would it make sense the mark a function as pure or constant? So that the compiler could know that passing the same parameters to the function will always return the same result.

@jellynoone
Copy link

Never mind. I see there is / was some progress being made at #46287

@bwilkerson bwilkerson added legacy-area-analyzer Use area-devexp instead. P3 A lower priority bug or feature request devexp-pkg-meta Issues related to package:meta and removed area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. pkg-meta labels Jul 18, 2022
@matanlurey
Copy link
Contributor

This was landed in b321254.

In the next Dart release (or sooner if you use an unstable release), the analyzer should trigger for @mustBeConst.

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. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants