Sdk: [package:meta] new annotation: @doNotStore

[pq]

Proposal: a new package:meta annotation @doNotStore to tag values that should not be stored in fields (or be returned from functions/getters that are not similarly tagged).

The annotation could be applied at the class, class member, top-level or library level.

/// Used to annotate a library, class, instance method, getter or top-level 
/// getter or function to indicate that the result of accessing it should not be 
/// stored in a field or top-level variable. If applied at the library or class
/// level, all library or class members will be treated as implicitly annotated.
/// If a value marked as `doNotStore` is returned from a function or getter,
/// that function or getter should be similarly annotated.
/// 
/// Tools, such as the analyzer, can provide feedback if
///
/// * the annotation is associated with anything other than a library, class,
///   instance method or getter or top-level getter or function, or
/// * an invocation of a member that has this annotation is returned by a method,
///   getter or function that is not similarly annotated as `doNotStore`
/// * an invocation of a member that has this annotation is assigned to a field
///   or top-level variable
const _DoNotStore doNotStore = _DoNotStore();

Corresponding issue: https://github.com/dart-lang/linter/issues/2085.

[pq]

@bwilkerson: I added fodder for conversation to the proposal. Input and word-smithing appreciated.

[pq]

As discussed, enforcing:

an invocation of a member that has this annotation is returned by a method, getter or function that is not similarly annotated as doNotStore

will have to be best-effort (in the absence of flow analysis) but I don't know that we need to hint at that in docs. Or?

[pq]

Is there any reason we shouldn't broaden this to all be applicable to all members? IOW,

~instance method, getter~ -> class member?

[bwilkerson]

... will have to be best-effort (in the absence of flow analysis) but I don't know that we need to hint at that in docs.

No. The docs should indicate the intended semantics for the annotation. If the tooling fails to fulfill that intent, then it's a bug. Keeping the annotation's documentation "pure" allows users to know when they've found a bug that impacts them.

Is there any reason we shouldn't broaden this to all be applicable to all members?

I don't think we want to annotate fields, top-level variables or setters, all of which I would include in the category of "members".

I thought of another hole: parameters. Given

@doNotStore
String get foo => ...;

class C {
  String field;

  void setField(String s) {
    field = s;
  }
}

void f(C c) {
  c.setField(foo);
}

the analyzer wouldn't have any way to detect that the value of foo was being stored in field. In order to cover this case we'll also need to be able to annotate formal parameters.

@jefflim-google Interested in your thoughts about this.

With that in mind, I think the documentation needs to be updated to something like the following:

/// Used to annotate a function (including methods, getters, top-level getters,
/// top-level functions and local-functions) or a formal parameter to indicate
/// that the value returned from the function, or the value of the parameter,
/// should not be
/// * stored in either a field or top-level variable,
/// * returned from a function that is not also annotated with this annotation,
///   or
/// * passed as an argument to any function unless the corresponding parameter
///   is annotated with this annotation.
///
/// This annotation can also be applied to a library or a class, in which case
/// it is implicitly applied to all of the functions (but not parameters) within
/// that library or class.
///
/// Tools, such as the analyzer, can provide feedback if
/// * the annotation is associated with anything other than a library, class,
///   function or parameter,
/// * an invocation of a function that has this annotation is returned by a
///   function that is not similarly annotated as `doNotStore`, or
/// * an invocation of a member that has this annotation is assigned to a field
///   or top-level variable.

[jefflim-google]

the analyzer wouldn't have any way to detect that the value of foo was being stored in field. In order to cover this case we'll also need to be able to annotate formal parameters.

This is already a problem, and not one I think we want to tackle. There will be valid reasons to store values into class vars, and even class finals. This could be a mistake, or it could be correct. I don't think we can easily decide from the static analysis PoV.

The biggest problem we're trying to address is the guaranteed mistake case of top level final or class level static final holding on to strings.

[bwilkerson]

I'm fine with not handling the case of parameters, but just to be clear ...

This could be a mistake, or it could be correct. I don't think we can easily decide from the static analysis PoV.

That's one of the main purposes for annotations: the ability to express user intent that isn't expressible via the language. If it were important enough (and I'm fine with saying that it isn't), then we could require that users state their intent everywhere so that we _can_ decide statically when their intent has been violated. However, doing so places a heavy burden on users so it's important that the value justifies the cost.

[jefflim-google]

Totally get it, sorry, I should have written a slightly longer reply :)

To do this, we'd either have to have a whitelist annotation or a blacklist annotation to everything receiving a @doNotStore. This would likely involve updating every single String parameter on whether it is safe or not, and I think the payoff would be small.

In most cases, if a value is stored, but not updated on language change, it's not the fact that it's stored that's a problem, but the fact that it isn't dependent upon the locale properly (i.e. that it isn't being then updated again).

Perhaps there's an easier way, but I don't feel this is an important enough case to tackle right now :)

[pq]

With @jefflim-google's comments in mind, I'm leaning towards not addressing formal parameters in the first cut (but leave the door open to refinement).

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

[pq]

Annotation landed in https://github.com/dart-lang/sdk/commit/37ba50cd0c838af2ececebf51ff3a67586df4be2 and will be published in the upcoming meta release. Analysis support being tracked in https://github.com/dart-lang/sdk/issues/42497.