dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

"is" operator inconsistent with type coercion for member variables #21530

Open DartBot opened 9 years ago

DartBot commented 9 years ago

This issue was originally filed by matthe...@gmail.com


What steps will reproduce the problem?

  1. Create an if statement with an is operator to test a member variable type against a sub-type.
  2. In the if block try to use the member variable as the coerced type.

What is the expected output? What do you see instead? Expected: Dart should coerce the member as it does with local variables and function parameters. Actual result: Dart editor produces an error and requires an explicit cast to use the type.

What version of the product are you using? Dart 1.6.0

On what operating system? Mac OSX

What browser (if applicable)? N/A

Please provide any additional information below. I would actually prefer Issue #5031 be addressed then an implicit coercion be used, but since the implicit coercion appears to be the preferred method in Dart it should be consistent.

kasperl commented 9 years ago

Added Area-Language, Triaged labels.

lrhn commented 9 years ago

Removed Type-Defect label. Added Type-Enhancement label.

lrhn commented 9 years ago

The problem with allowing   if (x is Foo) {     bar();     x.fooMethod();   } to type-check even if x is an instance getter, is that it might hide a bug. The "x" field may be typed as Object, and it might be set to something else during the "bar()" call, without breaking the contract of the object. If you call "x.fooMethod()", the warning you get correctly tell you that you can't be sure that "x" has a fooMethod. If you absolutely know that x won't change, you can easily change it to:   if (x is Foo) {     Foo foo = x;     bar();     foo.fooMethod();   } and avoid the warning (and also avoid any problems if x does in fact change).

DartBot commented 9 years ago

This comment was originally written by matthew1061...@gmail.com


That is a good point and another fair argument in favor of Issue #5031 as the pattern would remain consistent across all usages.

ex:

Foo foo = x as Foo; // <-- not possible with throw :( if(foo != null) {     bar();     foo.fooMethod(); }

Currently 'as' is basically useless at this point and actually could introduce a bug in the implicit / non-implicit scenario you have described.

consider:

if(x is Foo) {     bar();     (x as Foo).fooMethod(); }

With the current generic warning generated from the 'is' block with member usage; a programmer could very well think the above approach would fix the problem, but would yield a similar result as the bug you described.

In the end I would prefer casting to be more explicit regardless. There are valid situations when they are handy and necessary, but the programmer should pretty much always be cognizant of the usage and possible side effects. What if, for example, the user actually intends to call fooMethod on the current member and not a local variable as they know that bar may change said member. Maybe removing the reference was an intended side effect of 'destroying' the previous object.

The programmer would need to do another cast regardless in this case (any casting pattern will do):

if(x is Foo) {     bar();     if(x is Foo) // <-- Make sure we are still substitutable for a Foo     {         Foo foo = x;         foo.fooMethod();     } }

In the end casting is an explicit act and would preferably be used in that way.

I would think at least 1 of two resolutions could improve the situation a bit.

  1. Issue #5031
  2. Add more explicit error output on the member 'is' block to inform the developer 'Implicit cast of %MEM to %TYPE may have unintended side effects'
gbracha commented 9 years ago

We are looking to make type promotion more accepting, at the cost of further unsoundness.


Set owner to @gbracha. Added Accepted label.

srawlins commented 5 years ago

Duplicate of https://github.com/dart-lang/sdk/issues/34480 which, while opened later, has more discussion and examples.