feat/ Add @mustBeConst lint

[jeffkwoh]

Add a lint for dart parameters that allows the analyzer to check if parameters are const.

Sample Use cases

When parameters are compile time constants, it provides the guarantee that the parameters cannot be user supplied. This involves the overall security of our applications, and reduces the need for security and privacy reviews when code can be formally verified to be safe. This lead to the following use cases:

Designing API for SQL methods that are guaranteed against SQL injection

/// Any call to this method cannot suffer from SQL injection, since param1 and /// param2 must be const. If the strings are interpolated strings, the
/// parameters in the interpolated strings should be const as well.
void makeDbCall(@mustBeConst String param1, @mustBeConst String param2) {
  db.query("FROM DBLICIOUS SELECT $PARAM1, $PARAM2");
}

Logging libraries can ensure that there is no PII in log messages from prod

/// Any call to this method guarantees that there is no PII since const 
/// parameters cannot be user generated.
void logInfoNoPii(
  @mustBeConst String logMessage, 
  @mustBeConst DartObject object, 
  @mustBeConst Error error
) {
  logger.info(...);
}

• Dart SDK Version (dart --version)
  google's internal version

Internal link: b/190345386

[davidmorgan]

Another use case: it looks like I could use this to add const constructors to built_collection classes, which is an oft-requested feature.

const BuiltList.compileTime(@mustBeConst List<E> list) : _list = list;

Without the annotation this is incorrect because list might be mutable, in which case it needs to be copied.

[simolus3]

This would also be useful to denote functions that end up being sent across isolates. These have to be top-level or static, so effectively constant:

Future<T> compute(@mustBeConst FutureOr<T> Function() computation) {
  // computation will be sent over a port
}

Future<Isolate> spawn(@mustBeConst Function entrypoint) {
  // entrypoint will be passed to Isolate.spawn
}

[vsmenon]

Any updates here?

[simolus3]

I'm happy to work on this if no one else is yet.

[vsmenon]

@jeffkwoh is investigating with this internally ( b/190345386 ). If that goes well, we'll move it to the public linter.

[jellynoone]

Has there been any progress to this?

Additionally, I was wondering there have been any considerations made to also enable developers to mark a return of a functions as constant given the parameters passed to it being identical.

For instance consider the case of SQL where you might not define all of the queries beforehand but also build some more complex ones. In that case if a deep api function parameter is marked with @mustBeConst then you will also always have to manually build the result for all the possible queries.

Consider:

abstract class Db {
  dynamic execute(@mustBeConst String query, [Map<String, dynamic> parameters]);
}

class Helper {
  final Db _db;

  Helper(this._db);

  List<List<dynamic>> selectFields(
    @mustBeConst String table,
    @mustBeConst List<String> fields,
  ) {
    // this would indicate an error without List.join being able to indicate
    // that given a set of identical parameters the returned result is always
    // the same (not necessarily constant (in terms of const constructor))
    return _db.execute('SELECT ${fields.join(',')} FROM $table');
  }
}

To allow better composition it would then make sense to be able to indicate that the result of the function is always the same given identical parameters, which would probably indicated that it doesn't rely on non-constant global state or non constant internal state. Would @stableReturn be the right annotation or perhaps @pure?

This would also mean that the function would have to inherit the properties of the given input. So if we consider the following function example:

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

void safeLog(@mustBeConst String log) => print(log);

void main() {
  final log = join('[Info] ', 'Calling main');
  safeLog(log);
}

it should still work, since because both parameters passed to the join function were const and the function doesn't use any global state, the result should also be consider const not in a sense that it was created through the const constructor but that it was derived from const.

I know this is a different feature but I think they compliment each-other well.

[jellynoone]

After thinking about this some more, I realised that I was equating @mustBeConst to literal variables which isn't quite the same. I was for instance thinking that the mustBeConst parameters can also be combined together to produce analyser aware constants which aren't really constant in terms of const.

For instance, I was thinking this would be allowed:

void fn1(@mustBeConst String str, @mustBeConst String str2) => safeLog("$str$str2");

void safeLog(@mustBeConst String string) => print(string);

which if I understand the proposal correctly wouldn't be since our "$str$str2" is constant only at the analyser level, not language one.

But if this were allowed then we would essentially have two types of const values, one which are compile constants and the other which are also inferred constants. At which point, personally, I think a new name should be chosen, perhaps @mustBeLiteral where it could be tracked if the variable is literal through even composition.

So the previous example:

void fn1(@mustBeLiteral String str, @mustBeLiteral String str2) => safeLog("$str$str2");

void safeLog(@mustBeLiteral String string) => print(string);

would work because both str and str2 are literal which means concatenating them should also produce a literal string.

This way for instance the literal property could be tracked:

void fn(@mustBeLiteral String literal, String anyStr) {
  final str1 = 'prefix:$literal'; // is literal
  final str2 = '$literal$anyStr'; // is not literal
}

But I have to admit I have a hard time imagining what this would mean for datatypes other than String.

[rakudrama]

I have a use-case for our JavaScript templating feature

sdk/pkg/js_ast/lib/src/builder.dart

Lines 199 to 205 in 92972ff

	Expression call(String source, [var arguments]) {
	Template template = _findExpressionTemplate(source);
	arguments ??= [];
	// We allow a single argument to be given directly.
	if (arguments is! List && arguments is! Map) arguments = [arguments];
	return template.instantiate(arguments) as Expression;
	}

)

I want to ensure that most uses pass a constant as the source parameter, like this example which passes a string literal:

sdk/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart

Line 111 in 92972ff

final function = js('function(#) { return #.#(#); }', [

The js_ast library supports interpolation of JavaScript trees (filling in # with other trees) and should not be used in a way where the arguments are generated by interpolating strings. I can prevent this by requiring that the first argument is a constant.

Enforcing that something is a constant is a mechanism used to enforce a policy. There are two variables that I see in use cases.

1. The motivation for the restriction (aka the reason).
2. What constitutes a constant

I suggest that these are made to be parameters of a general MustBeConst annotation class. The users of the annotation would create an annotation constant for their own use that has clear guidance related to the policy.
For js_ast I would suggest something like this:

const useTreeInterpolation = MustBeConst(reason: r'''
Arguments to `js()` should be constants.
Constructing strings should be avoided. Use tree interpolation instead.
If building a string is unavoidable, use uncachedExpressionTemplate to avoid memory leaks.
''',
restrictions: [MustBeConst.literals, MustBeConst.variables]
);

used so:

   Expression call(@useTreeInterpolation String source, [var arguments]) { 

When the lint fires, the reason, if present, is included in the message to make it more actionable.

Regarding what constitutes a constant, I see several levels. For my specific case I would be happy with literals and references to declared const variables. I would hate this feature to be delayed further due to bike-shedding over the more complex versions, so lets make the design expandable and implement the simple stuff and add the other stuff later.

I see these tiers of constant complexity:

1. Primitive literals or const literals (e.g. const ["hello", "world"]) are required - this forces the value to be apparent at the call site, which may be desirable for a code review policy.

2. Any const expression is required - e.g. a reference to a const variable or const constructor is also allowed. These are obviously constant, but may be tedious since, say, "version $major.$minor" would have to be named.

3. Potentially constant expressions - expressions that will always have the same value. A non-constant expression is permitted provided all the variables used in the expression are constant, and the way they are used is limited to a fixed set of operations. This is the same idea as potentially constant expressions for const class field initializers. Perhaps the analyzer has this as an algorithm that could be called by the lint?

4. Depends only on constants - non-constant expressions are allowed provided they are 'constructive' over constant values. The value can be e.g. a non-constant list of literals. The extra expressions are enumerated somewhere, but include list, set and map literals, non-const calls to const constructors, and perhaps indexing of const lists and maps.

[lrhn]

Requiring something to be a constant can be done for different reasons.
Constants have three properties:

• Known at compile-time (good for macros and source-manipulations.)
• Immutable (can be safely stored without copying.)
• Canonicalized (saves memory, rarely an API concern, but I believe Flutter widget-change detection relies on identity.)

The use-cases listed here depend on one of the first two. The BuiltList example wants immutability, but should be satisfied by getting a new List.unmodifieable(...) created at runtime. It seems like more of an @mustBeUnmodifiable.

If you need deep immutability, it could be @mustBeImmutable, which is currently something we track on objects, but we could. (We could add an immutable modifier to types, and have a bit on every object which is known to be deeply immutable, so we can promote to that type at runtime. It's possible, but possibly not worth the complexity.)

Being known at compile-time is really only possible for constants, so @mustBeConst would be the right name for that.
It's also something that isn't particularly useful outside of code-generation, because that's the only code which sees the "values" before runtime. It might be better solved by something related to the macros feature.

[chingjun]

Slightly different from what is discussed above: a lint to make sure that all the instances of a given class must be const.

Use case: In Flutter, we tree shake icon fonts to remove the glyphs that are not being used in the application. And this is performed by scanning the tree-shaken dill file looking for const IconData. And if non-const instance of IconData is found, the icon font tree shaking step would fail.

It would be nice if we can add a @mustBeConst annotation on class IconData to make sure that it is always instantiated as a constant.

cc @christopherfujino

[matanlurey]

This was landed in b321254.

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