opened
srawlins
username_0
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:

```dart
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.
meta annotation request: sideEffects
issue
created
bwilkerson
username_1
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?
comment
created
srawlins
username_0
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:

```dart
// 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 (@username_6 I think analyzed internal code against the unnecessary_statements rule... and found very few affected intentional side effects).
comment
created
parren-google
username_2
To echo an internal thread, I 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.
comment
created
srawlins
username_0
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.
comment
created
natebosch
username_3
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.
comment
created
dskloetg
username_4
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.
comment
created
lrhn
username_5
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.
comment
created
davidmorgan
username_6
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.
comment
created
srawlins
username_0
I think @username_3 and @username_6 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 @username_6'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.
comment
created
srawlins
username_0
If no objections to a `justForSideEffects` in pedantic, I'll go ahead and close this in favor of that.
comment
created
davidmorgan
username_6
SG, let's discuss over there. Thanks!
comment
closed
srawlins
username_0
issue
created
srawlins
username_0
Opened https://github.com/dart-lang/pedantic/issues/13
comment