Sdk: meta annotation request: sideEffects

[srawlins]

I'd like a new class in the meta package, SideEffects, and a new top-level const instance of that class, sideEffects.

This fellow could be used to annotate a getter [1], indicating when a getter has side effects. Any static analysis rules (including lint rules), can then be written to "safely" assume that any property access or prefixed identifier has no side effects. I say "safely" because any time a confused developer sees a static analysis report and says "oh that's not right at all," they can add this @sideEffects annotation to a getter, which the rule can use to not generate false positive reports.

For example:

class A {
  @sideEffects
  B get b {
    print('b was gotten');
    return B();
  }
}

class B {
  int c, d;
}

void f() {
  if (a.b.c == null) {
    a.b.c = 7; // triggers a prefer_conditional_assignment lint report.
  }

  a.b
      ..c = 1
      ..d = 2;
  a.b.c = 3; // triggers a cascade_invocations lint report.

  a.b; // triggers unnecessary_statement lint report.

  a.b.c = 7;
  return a.b.c; // triggers a join_return_with_assignment lint report.
}

Each of these four lint reports are incorrect, as get A.b has side effects. We've seen real code in, e.g. caching modules that have to ignore these reports.

[1] why just a getter? Meh, I'd be happy to open this up to more node types, but I've only thought of uses for getters for now.

[bwilkerson]

I have to question the value of this. Even if this annotation existed, I don't think analyzer could reasonably assume that all existing code had been updated to mark all getters that have side effects with this annotation. And no, users can't necessarily add the annotation because they might not own the code to which the annotation would need to be added.

How often are users running into this issue?

[srawlins]

I think the only case where we've run into this in real life is when examining unnecessary_statements for internal use. I haven't seen specific calls for this otherwise.

If this annotation existed, then anyone running into a false positive in their own code, wanting to avoid it, has a path to success: add the @sideEffects annotation to the appropriate code, or file a bug or a PR against the appropriate code if you don't own it. If you are unable to immediately apply the fix, you can add a comment to your own code:

// TODO(user): Remove this directive when `bar.getThing` is marked as having @sideEffects.
// https://github-url
// ignore: unnecessary_statements
bar.getThing;

And that comment can be removed in your code and other code which calls bar.getThing, once the annotation is applied.

It would be a shame to block helpful analysis (in, for example, unnecessary_statements, join_assignment_with_return, cascadable_invocations, prefer_conditional_assignment) for the <1% of getters with side-effects (@davidmorgan I think analyzed internal code against the unnecessary_statements rule... and found very few affected intentional side effects).

[parren-google]

To echo an internal thread: the concept of a side-effect is somewhat debatable. For example, is pure memoization on top of immutable values a side-effect? To most users it's not, and annotating a getter with @sideEffects would feel wrong. But it does have a performance impact for subsequent calls.

Or consider something like MobX, where memoization leaves an outside trace of it being a dependent of other values.

It would also have to be transient. If I depend on something that has @sideEffects, I have them too.

Overall, I think an // ignore comment with a proper justification at the call site is better than this annotation at the source.

[srawlins]

I'm also fine with the idea of having a policy in the linter that says all property access is assumed to be pure, without side effects, and users just have to // ignore certain lint warnings when its not pure. I know there is a lot of opposition thought to lint rules with known consistent false positives and having // ignore comments as part of the plan and standard procedure for using a rule.

[natebosch]

To make sure we're all on the same page:

The request is not to make @sideEffects mandatory, only to allow it to be a marker of the getters which some callers may be using for it's side effects. It only needs to be applied transitively if some code is calling that getter for it's transitive side effects. It would still be acceptable to have a getter with side effects (transitive or otherwise) and not use this annotation.

I do think that the definition is the right place to document this. It's an expression that the API is _intended_ to be used for it's side effects. If the author did not intend for the getter to be called for it's side effects then an // ignore at the call site is an indication that the code could be broken upstream because it's relying on implementation details.

We currently (hopefully) express the side effects of a getter in the doc comment. An annotation that the linter can understand seems like a strict improvement to me.

users can't necessarily add the annotation because they might not own the code to which the annotation would need to be added.

That's certainly true but I don't think it should stop us from using it on the code where we can control whether the annotation is added.

[dskloetg]

If I saw this annotation with the proposed name I would assume that the analyzer would fail on getters with a side effect that don't have the annotation. If that's not the intent, I think it should have a different name.

[lrhn]

SIde-effects in a getter is an anti-pattern (most people assume they won't modify the object or mutable state), so why not just discourage it even more, rather than enable it like this?

I'd rather change the getter to a function with a name that documents its side-effect, than have even more annotations to mark this as a deliberate bad design.

[davidmorgan]

When we looked at getters across google3 in the context of unnecessary_statements, we did find getters with side effects, but almost exclusively that side effect was to do with loading or caching, i.e. not visible to the caller.

The obvious question is, if the side effects are not visible, why would you want to trigger just the side effect in a way which would trigger a lint? The answer, which we did see in practice, is for precaching/loading, testing and benchmarking.

As Sam mentions there is a general feeling that to get the most value from lints we should have it be possible to have them be enforced 100% without needing ignore.

At first glance I don't think @sideEffects is quite right for that; I think that it's usually a property of the _caller_ that a getter is being used just for the side effect--as mentioned, for precaching, testing or benchmarking.

So how about a justForSideEffects method, similar to unawaited, which would live in package:pedantic? That would be enough to silence unnecessary_statements, not sure about the others.

[srawlins]

I think @natebosch and @davidmorgan make good points that it is not common to _depend_ on the side effects of a rare getter with side effects. So the cases we saw were not that a caller always depended on a getter for its side effects, but that, in a small number of case, it did. So the only times a bar.getThing; is called on its own in a statement, is during a test of that side effect.

In which case I like @davidmorgan's suggestion to put a justForSideEffects in pedantic, and document that (all, some?) linter rules assume that getters don't have "externally meaningful" (TBD) side effects, and that they will report Lint that may reduce the number of calls to property access, assuming it has no side effects.

[srawlins]

If no objections to a justForSideEffects in pedantic, I'll go ahead and close this in favor of that.

[davidmorgan]

SG, let's discuss over there. Thanks!

On Wed, 9 Jan 2019 at 23:12 Sam Rawlins [e-mail] wrote:

If no objections to a justForSideEffects in pedantic, I'll go ahead and
close this in favor of that.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/35513#issuecomment-452888427,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFR8mMYeKUAQ2XLU5QFwQfJty01TmCK7ks5vBmlQgaJpZM4ZkPo1
.

[srawlins]

Opened https://github.com/dart-lang/pedantic/issues/13