Skip to content

Base class type inference is wrong #30079

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

Closed
jacehensley-wf opened this issue Jul 5, 2017 · 17 comments
Closed

Base class type inference is wrong #30079

jacehensley-wf opened this issue Jul 5, 2017 · 17 comments
Labels
legacy-area-analyzer Use area-devexp instead.

Comments

@jacehensley-wf
Copy link

jacehensley-wf commented Jul 5, 2017

See this reduced test case:

Simple: https://dartpad.dartlang.org/6ee6ea9b7f00bc1b6c7115a891ba88b8

How it was discovered in OverReact components: https://dartpad.dartlang.org/e5e40dd06c6beee77077773bc5880cc6

This issue was found in OverReact

I would expect classInstance to have a type of AbstractComponent

Only an issue is TypedBase implement Interface or if the generic params are left in.

This is in strong mode.

@bencampbell-wf
Copy link

It appears that simply implementing any class in the example leads to the same analyzer error. They do not need to overlapping symbols or implementations.

I have updated the supplied example to illustrate this https://dartpad.dartlang.org/1c8aff38857a3cb5aa12f142d7b6ce67 .

@robbecker-wf
Copy link

@leafpetersen @kevmoo Is this expected behaviour? This seems to be an analyzer bug.

@kevmoo kevmoo added the legacy-area-analyzer Use area-devexp instead. label Jul 5, 2017
@kevmoo
Copy link
Member

kevmoo commented Jul 5, 2017

@bwilkerson @devoncarew ?

@bwilkerson
Copy link
Member

The problem is the definition of LUB, which comes into play with the ternary "?:" operator. I don't know whether inference can / will do better in the future, but for now the only work-around I know of is to explicitly give classInstance a type.

@jacehensley-wf
Copy link
Author

@bwilkerson Why would adding an implements to TypedBase break this? without the implements it works correctly.

@robbecker-wf
Copy link

Also, if we have to live with this, can I request a lint rule to remind people to specify a type instead of var when assigning using a ternary?

@aaronlademann-wf
Copy link

aaronlademann-wf commented Jul 5, 2017

Also, seems like a bug - if for no other reason - its not consistent.

If you remove the generic parameters from the OverReact example linked above - the type inference is suddenly correct: https://dartpad.dartlang.org/36c92bfa01f3903606b7aa4328b2dfcd

For the sake of posterity:

Analyzer Error:

main() {  
  // Analyzer thinks this is `Object`, it should be `AbstractComponent`.
  var classInstance = (someCheck() ? new ComponentA() : new ComponentB());

  classInstance.baseMethod();
}

bool someCheck() {
  return false;
}

class AProps {}
class AState {}

class BProps {}
class BState {}

class ComponentA extends AbstractComponent<AProps, AState> {}

class ComponentB extends AbstractComponent<BProps, BState> {}

class AbstractComponent<T, S> extends UiStatefulComponentTransformerHelper<T, S> {}

class UiStatefulComponentTransformerHelper<T, S> 
    extends UiStatefulComponent<T, S> with GeneratedClass {}

class UiComponentTransformerHelper<T> extends UiComponent<T> with GeneratedClass {}

class UiStatefulComponent<T, S> extends UiComponent<T> {}

class UiComponent<T> extends Component implements DisposableManagerV3 {}

class Component {
  baseMethod() {}
}

class DisposableManagerV3 {}

class GeneratedClass {}

Analyzer Clean:

main() {  
  // Correctly inferred as `AbstractComponent`
  var classInstance = (someCheck() ? new ComponentA() : new ComponentB());
  
  classInstance.baseMethod();
}

bool someCheck() {
  return false;
}

class ComponentA extends AbstractComponent {}

class ComponentB extends AbstractComponent {}

class AbstractComponent extends UiStatefulComponentTransformerHelper {}

class UiStatefulComponentTransformerHelper extends UiStatefulComponent with GeneratedClass {}

class UiComponentTransformerHelper extends UiComponent with GeneratedClass {}

class UiStatefulComponent extends UiComponent {}

class UiComponent extends Component implements DisposableManagerV3 {}

class Component {
  baseMethod() {}
}

class DisposableManagerV3 {}

class GeneratedClass {}

@jacehensley-wf
Copy link
Author

jacehensley-wf commented Jul 5, 2017

@bwilkerson Is this related to #27525?

And this: #27922

And maybe this: #24202

@jmesserly
Copy link

yes this is #27525 and #25821, LUB does not work as expected in Dart for many types, but especially generic types.

@jacehensley-wf
Copy link
Author

Okay, I'm guessing those improvements are not slotted for the near future? Do they have a rough ETA or a hopeful SDK version?

@jmesserly
Copy link

ah, I'm not sure. @leafpetersen might know the status

@leafpetersen
Copy link
Member

As others have said, least upper bound is a disaster in Dart, and it's really hard to fix without substantially changing the language. The smaller reproduction example above is one of the classic examples: you have a hierarchy that looks like:

A -> Sub1 -> Base, Element -> Object
B -> Sub2 -> Base, Element -> Object

There simply is no least upper bound for this hierarchy, period: neither Base nor Element is least. Object is very much not least, but at least it is unique, and that's what the Dart LUB definition chooses.

The more complex examples above fall down in other corners of the LUB definition around generics, but it's a similar situation, with the addition that you run into decidability issues (if you try to compute an upper bound recursively, there are examples which loop around to the original query, or worse to an infinitely expanding series of queries).

I would still like to improve on this, but it is unlikely to happen soon.

@leafpetersen
Copy link
Member

Also, if we have to live with this, can I request a lint rule to remind people to specify a type instead of var when assigning using a ternary?

I would be very happy to see this. I would suggest including things like a ?? b and a ??= b in there as well, since I believe both of those (or maybe just the former) also have the same problem. Could you maybe file a separate issue for this?

@leafpetersen
Copy link
Member

I'm going to close this, since issues with LUB are well tracked elsewhere. Feel free to reopen if you feel that there is something particular to track here.

@jacehensley-wf
Copy link
Author

Thanks for the responses everybody 👍

@leafpetersen
Copy link
Member

Some related other issues:

#25368
#25821
#27525

@robbecker-wf
Copy link

@leafpetersen I filed an issue for the requested lint rule here #57606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

8 participants