Skip to content
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

Unexpected unnecessary_cast reported when variable is reassigned later #48984

Closed
DanTup opened this issue May 9, 2022 · 11 comments
Closed

Unexpected unnecessary_cast reported when variable is reassigned later #48984

DanTup opened this issue May 9, 2022 · 11 comments
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@DanTup
Copy link
Collaborator

DanTup commented May 9, 2022

final strings = <String>[];
var stringIterable = strings as Iterable<String>; // unnecessary_cast
if (foo)
  stringIterable = stringIterable.where((_) => x.isNotEmpty);
if (bar)
  stringIterable = stringIterable.map((x) => '$x$x');

This code reports unnecessary_cast on casting a List to Iterable, but without it you can't re-assign the variable if you want to append some filters.

Writing Iterable<Strings> strings = <String>[]; to avoid writing the cast works fine.

@eernstg
Copy link
Member

eernstg commented May 9, 2022

I guess the topic here is that the cast isn't actually unnecessary, because it's needed in order to guide the inference of the declared type of stringIterable.

However, it could be claimed that the direct solution (Iterable<String> stringIterable = strings;) is better than the one where the inference is nudged in the desired direction, because (1) this serves to document the intended typing directly, (2) it's safe (in the sense that it's a compile-time error if it isn't an upcast, but as T would be allowed for any T).

Using Iterable<String> strings = []; would work just fine as well (assuming that there's no need to use the typing of strings as List<String>).

So perhaps it's not a problem that unnecessary_cast flags this situation?

@DanTup
Copy link
Collaborator Author

DanTup commented May 9, 2022

Perhaps my interpretation of "unnecessary" is not what was meant by the message. To me, it seemed to suggest "this cast can be removed and everything will work the same".

Even if the warning is correct though, it feels odd to me that the code has a quick-fix (which removes the cast) that results in code with an error. After doing that, it's perhaps not very clear what the correct action is that avoids bringing back the warning.

If the suggestion is to move the type to the left, it might be nice if the fix could do that instead of only removing the cast. Whether that's feasible (or even worthwhile), I'm not sure.

@eernstg eernstg added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-warning Issues with the analyzer's Warning codes labels May 9, 2022
@eernstg
Copy link
Member

eernstg commented May 9, 2022

@bwilkerson, what do you think? First, it looks like a false positive to me, too, when unnecessary_cast flags the cast, even though it does have an effect.

Could unnecessary_cast be adjusted such that this situation is recognized (perhaps by a new hint/lint, or perhaps via a different behavior of unnecessary_cast in that case)? It would apply when a variable declaration var v = e as T; (local or not) is such that as T is an upcast (perhaps: and T is not dynamic). It would have a quick-fix that turns it into T v = e;.

@bwilkerson
Copy link
Member

The unnecessary_cast warning is intended to be a form of dead code detection, so if removing the cast doesn't preserve semantics then it's a false positive.

We could certainly add a lint that would catch this case and recommend using the style of adding an explicit type annotation rather than using the cast to influence inference. I personally think that's a better style because it's more explicit and it avoids an unnecessary run-time check (assuming the compiler doesn't optimize the check away), but it's a question of stylistic preference so it should be opt-in.

@keertip keertip added type-enhancement A request for a change that isn't a bug P3 A lower priority bug or feature request labels May 9, 2022
@srawlins
Copy link
Member

srawlins commented Oct 17, 2022

I've seen this come up more w.r.t. null safety code. I think perhaps because the migration tool sometimes adds casts.

Anyway the only way I can see keeping this Hint "false positive"-free, is to stop reporting upcasts. See #49550, #49269, and #44411 for more examples.

I think the general idea is that it would be great to report an upcast as unnecessary if that type does not result in a change to an inferred type or a promoted type (beyond the left side of the as-expression and beyond the type of the as-expression itself). This is complicated to know; we don't track provenance of types which bubble up from inference.

I like @eernstg 's idea (which @bwilkerson also approved of, above) of a second rule, a lint, which would catch unnecessary upcasts in some declarations with initializers. We would still not catch all technically unnecessary casts, like

class A {}
class B extends A {}
void f() {
  var x = [B() as A];
  x.add(B());
  // There was no reason to cast.
}

but I think that's a very pragmatic approach. I don't think we can catch all assignment expression cases:

var x = [
   if (foo)
    ...[
      // Perhaps all of the other elements are `List<String>`, so this one cast changes the type
      // of `x`. It would be great to track that this cast could be replaced by a type annotation
      // on `x`, but difficult.
      if (bar) a as Iterable<String>,
      // ...
    ],
  // ...
];

But the common seem to be: assigning a ConditionalExpression where one branch is an AsExpression, assigning an AsExpression, and assigning an if-null expression where one branch is an AsExpression.

@srawlins
Copy link
Member

I requested said lint rule here: #58903

@bwilkerson
Copy link
Member

But the common seem to be: assigning a ConditionalExpression where one branch is an AsExpression, assigning an AsExpression, and assigning an if-null expression where one branch is an AsExpression.

Would those cases be sufficient to cover the false positives, or are they merely representative? If they are sufficient, then perhaps the cases we allow should be limited to those.

As I said elsewhere, my concern is that we could be too aggressive and stop reporting places where the cast is clearly not necessary in our attempt to eliminate all false positives.

@srawlins
Copy link
Member

Would those cases be sufficient to cover the false positives, or are they merely representative? If they are sufficient, then perhaps the cases we allow should be limited to those.

We already allow unnecessary casts in a conditional operation. See the test cases here: https://github.com/dart-lang/sdk/blob/main/pkg/analyzer/test/src/diagnostics/unnecessary_cast_test.dart#L22

Allowing unnecessary casts in conditional operations has not fixed this issue, #49269, #44411, or #49550.

Removing the unnecessary_cast from every upcast is very aggressive, but I believe necessary. As I stated above, we don't track the provenance of type promotion. In the example below that statement, you can see that an as-expression may be used in upwards inference for a list literal, which is then used in upwards inference for another list literal, which is then used to infer the type of a local variable, which may then be used to infer... etc.

@natebosch
Copy link
Member

Would it be possible to go in the other direction, and have the quick fix put in the type information that would be inferred due to the cast?

I don't know if we've had a wide discussion about it, but I do think that adding explicit types is preferred over nudging inference with casts.

@bwilkerson
Copy link
Member

Yes, the fix can definitely add explicit types (in all the cases that I can think of).

@srawlins
Copy link
Member

Apologies for the late reply.

I don't think we can simply follow what inferred types are influenced by an as-cast. Consider the example above, slightly modified here:

var /* COULD ADD Set<Iterable<String>> */  x = /* COULD ADD <Iterable<String>> */{
   if (foo)
    .../* COULD ADD <Iterable<String>> */[
      // Perhaps all of the other elements are `List<String>`, so this one cast changes the type
      // of `x`. It would be great to track that this cast could be replaced by a type annotation
      // on `x`, but difficult.
      if (bar) a as Iterable<String>,
      // ...
    },
  // ...
];

It would be a complex mechanism, which would use heuristics to decide where to annotate, for little payoff. And it would be enforcing one style.

I don't know if we've had a wide discussion about it, but I do think that adding explicit types is preferred over nudging inference with casts.

Whether or not we are, that is an opinion about style. For this static warning (not lint, not opt-in), we should not be requiring users to change their code from one style to another, to avoid a default-enabled static warning. I am fully in favor of re-implementing that behavior in the requested lint rule, #58903.

@jamesderlin gives an example where using a full record type at variable declaration becomes "awkward, tedious, and potentially error-prone," with:

(int, num, int) record = (1, 2, 3);

(obviously, imagine types with more than 3 letters, and records with more than 3 fields.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. 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