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

Analyzer does not check types correctly when create a List object #34486

Closed
iarkh opened this issue Sep 17, 2018 · 6 comments
Closed

Analyzer does not check types correctly when create a List object #34486

iarkh opened this issue Sep 17, 2018 · 6 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. closed-as-intended Closed as the reported issue is expected behavior needs-info We need additional information from the issue author (auto-closed after 14 days if no response)

Comments

@iarkh
Copy link
Contributor

iarkh commented Sep 17, 2018

Dart SDK Version: 2.1.0-dev.4.0
OS: Windows 10 (64 bit)

The following test example creates creates a List<List> object and assigns it to the List<List<int>> variable:

main() {
  List<List<int>> l1 = new List<List>();
}

Dart fails on this with type error, sample output is:

Unhandled exception:
type 'List< List< dynamic>>' is not a subtype of type 'List< List< int>>'
#0 main (file:///D:/DART/sdk/tests/co19/src/LanguageFeatures/Instantiate-to-bound/List/test.dart:2:19)
#1 _startIsolate. (dart:isolate/runtime/libisolate_patch.dart:289:19)
#2 _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:171:12)

The problem is that the same example unexpectedly passes with dartanalyzer, no errors when I run dartanalyzer test.dart command with this.

It seems like this result is incorrect as it's statically clear that 'List< List< dynamic>>' is not a subtype of type 'List< List< int>>'.

Also it's interesting that the following similar source code correctly fails both with analyzer and dart:

class A<X> {}

main() {
  A<List<int>> l1 = new A<List>();
}

Analyzer sample output looks correctly here:

Analyzing test.dart...
error - The constructor returns type 'A<List>' that isn't of expected type 'A<List>' at test.dart:4:21 - strong_mode_invalid_cast_new_expr
hint - The value of the local variable 'l1' isn't used at test.dart:4:16 - unused_local_variable
1 error and 1 hint found.

@eernstg
Copy link
Member

eernstg commented Sep 17, 2018

This is actually working as intended: The initialization of l1 contains a downcast, and that downcast fails at run time, so the program should be accepted by the analyzer and rejected at run time by the VM.

The tricky part here is that our tools may in some cases use a notion of an exact type (see #33307) to conclude that a particular downcast is guaranteed to fail at run time. We do not have a specification of exact types at this point (#33307 was created in order to make that happen), so you could say that it is a bug to use that concept at all—but I expect that we will specify exact types and we will make it an error to have some (maybe all) of these downcasts which are guaranteed to fail due to the exactness of some of the types in the given situation.

This means that it would not be useful to start removing this behavior from the tools where they currently have it.

In the example you show, it looks like the analyzer ascribes new A<List>() the static type A<List<dynamic>> as an exact type (that is, its dynamic type is guaranteed to be A<List<dynamic>>, it's not a proper subtype of A and its type argument is not a proper subtype of List<dynamic>) whereas new List<List>() does not get an exact type. The latter could be because it invokes an external factory constructor rather than a generative one. Given that this is a built-in declaration it would be possible for tools to ascribe an exact type even to such an external constructor, but it would hardly be a portable behavior because different platforms might behave differently in this respect (and we probably don't want to have the names of private built-in classes occur in error messages etc.).

So the "problem" is simply that new A<List>(); does get an exact type, even though no such concept has been specified, and the solution is to allow that without complaints at this point. ;-)

@matanlurey
Copy link
Contributor

I think it is fair to close this as a duplicate of either:

@eernstg

@eernstg
Copy link
Member

eernstg commented Sep 17, 2018

Maybe the most precise approach would be to close this issue because it requests a behavior which cannot (reasonably) be expected, and not actually because it's a duplicate of anything: It requests that List<List>() be given an exact type.

If we were to give new List<List>() an exact type then we'd need to let exact types originate from an external factory constructor. OK, implementations could do that because they can do whatever they want with built-in declarations, but I suspect that it would be considered bad style to let types like JSArray<List<dynamic>> and other "secret built-in" types pop up in error messages, and it wouldn't generalize to other factory constructors anyway, because they can return a proper subtype of the enclosing class. With a redirecting factory constructor that ultimately redirects to a generative constructor, instance creations could be given an exact type, but it is probably also a bad idea to consider that piece of information as a part of the public interface.

@iarkh, are you happy with that resolution?

@matanlurey matanlurey added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. needs-info We need additional information from the issue author (auto-closed after 14 days if no response) labels Sep 17, 2018
@iarkh
Copy link
Contributor Author

iarkh commented Sep 18, 2018

Well, so am I correct that test with List<List<int>> l1 = new List<List>(); case should pass without static compile error and A<List<int>> l1 = new A<List>(); should cause analyzer error?

Is there a way to detect which behavior is expected for other cases (i.e. functions, etc.?)

@eernstg
Copy link
Member

eernstg commented Sep 18, 2018

Right, List<List<int>> l1 = new List<List>(); has no compile-time errors (but will fail at run time). For A<List<int>> l1 = new A<List>(); we should not have an analyzer error according to the specification (because it knows nothing about exact types), and this is an unfortunate but known discrepancy, but I think we should allow tools to use exact types and emit these errors for now, and then we'll hopefully soon get it specified such that we can settle all the corner cases.

There is no systematic way to determine which expressions have an exact type because we simply haven't specified that concept. We do have an upper bound, of course, because the static type of an expression whose value at run time can actually be a proper subtype of the statically known type can never soundly be exact. It's just that some expressions may be guaranteed to have a specific exact type, and we haven't decided whether we'd detect that:

class A { const A(); }
class B implements A {}
const a = A();

main() {
  foo() => a;
  B b = foo(); // Error?
}

A local function gets its return type by inference if it is omitted, and the returned value for foo has an exact type (presumably, a constant is one of the most obvious candidates to have an exact type). So do we infer that the return type of foo is exactly A? In that case the downcast to B could be an error.

How much do exact types affect your work at this point? Is it necessary to exercise them? I'd expect that we will have a whole subtopic on testing exact types when we get that topic settled, but we do need to clarify the concept first.

@eernstg eernstg added the closed-as-intended Closed as the reported issue is expected behavior label Sep 28, 2018
@eernstg
Copy link
Member

eernstg commented Sep 28, 2018

@iarkh, it looks like we should close this issue with 'working as intended', so I'll do that. Please create a new issue to handle topics of this issue that you think we have not yet resolved, if needed.

@eernstg eernstg closed this as completed Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. closed-as-intended Closed as the reported issue is expected behavior needs-info We need additional information from the issue author (auto-closed after 14 days if no response)
Projects
None yet
Development

No branches or pull requests

3 participants