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

Some important packages fail to compile with Fasta #31756

Closed
lexaknyazev opened this issue Jan 3, 2018 · 11 comments
Closed

Some important packages fail to compile with Fasta #31756

lexaknyazev opened this issue Jan 3, 2018 · 11 comments
Assignees
Labels
front-end-fasta legacy-area-front-end Legacy: Use area-dart-model instead. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@lexaknyazev
Copy link
Contributor

yaml, pool, and test packages fail to run with "Dart 2" VM. I'm not sure whether the following issues are caused by the compiler or packages sources should be updated.

Dart VM version: 2.0.0-edge.2e3a4f87236a24dc120f6b29e8b37a3772287ad3 (Wed Jan 3 15:40:36 2018 +0000) on "linux_x64"
$ git clone https://github.com/dart-lang/yaml.git
$ cd yaml
$ ~/dart-sdk/sdk/out/ReleaseX64/dart-sdk/bin/pub get
Resolving dependencies...
Overriding the upper bound Dart SDK constraint to <=2.0.0-edge.2e3a4f87236a24dc120f6b29e8b37a3772287ad3 for the following packages:

analyzer, args, async, barback, boolean_selector, charcode, cli_util, collection, convert, crypto, csslib, front_end, glob, html, http, http_multi_server, http_parser, io, isolate, js, kernel, logging, matcher, meta, mime, multi_server_socket, node_preamble, package_config, package_resolver, path, plugin, pool, pub_semver, shelf, shelf_packages_handler, shelf_static, shelf_web_socket, source_map_stack_trace, source_maps, source_span, stack_trace, stream_channel, string_scanner, term_glyph, test, typed_data, utf, watcher, web_socket_channel, yaml

To disable this you can set the PUB_ALLOW_PRERELEASE_SDK system environment variable to `false`, or you can silence this message by setting it to `quiet`.
Got dependencies!

$  ~/dart-sdk/sdk/out/ReleaseX64/dart-sdk/bin/dart test/yaml_test.dart
00:00 +0: has a friendly error message for using a tab as indentation
00:00 +1: has a friendly error message for using a tab not as indentation
00:00 +2: refuses invalid contents
...
00:00 +146: All tests passed!

$ ~/dart-sdk/sdk/pkg/vm/tool/dart2 test/yaml_test.dart
lib/src/yaml_node_wrapper.dart:111:12: Error: Can't infer the type of '[]=': overridden members must all have the same type.
Specify the type explicitly.
  operator []=(int index, value) {
           ^
file:///home/alexey/.pub-cache/hosted/pub.dartlang.org/test-0.12.29+1/lib/src/runner/load_suite.dart:90:18: Error: The method 'close' isn't defined for the class 'dart.async::FutureOr<#lib1::RunnerSuite>'.
Try correcting the name to the name of an existing method, or defining a method named 'close'.
          suite?.close();
                 ^
file:///home/alexey/.pub-cache/hosted/pub.dartlang.org/pool-1.3.3/lib/pool.dart:125:22: Error: A value of type 'dart.async::Future<dart.async::Future<#lib1::Pool::withResource::T>>' can't be assigned to a variable of type 'dart.async::Future<#lib1::Pool::withResource::T>'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.async::Future<#lib1::Pool::withResource::T>'.
    return request().then<Future<T>>((resource) {
                     ^

The first error could be "fixed" by specifying void return type on operator []=, the third one seems to be similar to #31743.

/cc @mraleph

@a-siva a-siva added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-fasta labels Jan 3, 2018
@lexaknyazev
Copy link
Contributor Author

pool seems to be fixed as of 1.3.4.

@kmillikin
Copy link

You are right that the workaround for the first issue is to specify the return type void somewhere. I think you should be able to do that in class YamlListWrapper which will disable top-level type inference for the return type of that method, or you can do it in class YamlList which will allow type inference for the method in YamlListWrapper but give the algorithm a type it can infer.

@leafpetersen @eernstg @stereotype441

I'd like to make sure that we are implementing the intended behavior for the first issue. Here's the situation:

  • The implementation of operator []= in class YamlListWrapper does not have a specified return type, so top-level type inference attempts to infer one.
  • YamlListWrapper implements ListBase from dart:core which has a specified return type of void.
  • YamlListWrapper implements YamlList which has an inferred return type dynamic.
  • The PR for the spec of top-level inference https://github.com/dart-lang/sdk/blob/8f2be46aac7705ab6a1024a795aaf977519a8be2/docs/language/informal/toplevel-inference.md does not (currently) say what to do for the return type in this case, but we might infer ( ;) ) that we should treat it the same as the parameters, thus requiring the types to be equal.
  • void and dynamic are not equal, thus we make this a compile-time error.

I talked to @lrhn and he suggested that operator []= is like a setter so we might perhaps add a special case which would have inferred a return type of void for the method in YamlList, which addresses this specific case but not the general one.

Do we really want the requirement that the all the types are equal? Since we've (as far as I understand) decided that classes can inherit multiple instantiations of the same type, I think this will make using type-inference on those inherited methods a compile-time error.

@kmillikin
Copy link

The second issue, the one in package test, is a bug in our implementation. I'm working on a fix.

@kmillikin kmillikin self-assigned this Jan 5, 2018
@kmillikin kmillikin added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Jan 5, 2018
@lrhn
Copy link
Member

lrhn commented Jan 5, 2018

If a class C does not declare a member x, and one or more of its immediate super-interfaces includes a signature for a member named x, and one of those super-interface signatures is a valid override of all the other such signatures, then the interface of C will have that signature for x(1). If there is no signature that can override all the others, it's a compile-time error. (If there is more than one then ... well, pick one, say ... rolling dice ... the one from the syntactically latest declared of the immediate super-interfaces that had a such a declaration).

(1): ... and if any of the super-interface signatures of x have a covariant parameter, then the corresponding parameter of x in C will also be covariant.

@kmillikin
Copy link

In this case, the class YamlListWrapper does declare a member operator []= and we are trying to infer a return type for it.

@stereotype441
Copy link
Member

I like the idea of inferring the return type of operator []= in the same way we infer the return type of a setter; namely, if it is not specified the inferred type is always void. I filed #31779 to address it.

@kmillikin
Copy link

I have a CL that fixes the second problem, the one with package test, at https://dart-review.googlesource.com/c/sdk/+/32620. I'll update this issue when it lands.

@leafpetersen
Copy link
Member

The question of what to do when you inherit "equivalent" but not "equal" types is irritating. We could choose an ordering on the the simple types (e.g. prefer void over Object over dynamic), and maybe just punt (error out) on things like FutureOr<dynamic>. We still have the question then of what to do with something like inheriting (void, dynamic) -> dynamic and (dynamic, void) -> dynamic. Either we synthesize a new type (e.g. (void, void) -> dynamic), or we pick an arbitrary one, or we error out. I'm not totally averse to synthesizing a type in this case, but we have tried to avoid it.

@lrhn
Copy link
Member

lrhn commented Jan 6, 2018

For equivalent-but-not-equal types, consistently picking any of them is likely good enough. If you always pick the syntactically last one, you can rearrange to put the interface of the one you want at the end (unless it's the one from the superclass, then you're out of luck). For a normal class declaration you can add an abstract override in the body. For a mixin application class, you can add an interface if necessary.
For a mixin-application-superclass, you're again out of luck - you'll have to extract that class and give it a name so you can add an extra interface. But again, it likely won't happen, or not matter, in practice.

@eernstg
Copy link
Member

eernstg commented Jan 8, 2018

We have certain significant distinctions between types that are equivalent according to the subtype relation, and then we also have some accidental ones.

In particular, general rules for union types justify conclusions such as FutureOr<Object> == Object (because the former is of the form "Something U Everything" which is "Everything"), and the newly introduced rules in the language specification also ensure that this equivalence can be proven. These distinctions are accidental in the sense that we might as well consider Object and FutureOr<Object> to be different spellings for the same type, and there is no good conceptual reason for wishing to do anything other than normalizing all these types such that we agree on one, "best" spelling for each of them. We may or may not wish to establish the formal guarantees ensuring that these types are always normalized, but ideally they would be.

In contrast, void differs from Object by flagging usages (because expressions with that type are expected to yield values of no interest), and dynamic differs from Object by allowing dynamic member lookups. In both cases the difference from Object has nothing to do with subtyping, it's directly concerned with the treatment of expressions of that type, and these distinctions can generally be assumed to be intended when declared by developers, and maybe even when obtained implicitly. Similar considerations apply for composite types like List<dynamic> and void Function(). These distinctions should not be normalized away, they are significant.

It's tempting to introduce a "micro structure" into the type system and say something like "Object <: dynamic <: void", hoping that this could give is reasonable specificity rules for computation of class interfaces (so Object always wins in a covariant position, and void always wins in a contravariant position, and dynamic only persists when it's alone), but it's not obvious to me that this is sufficiently pragmatic (i.e., that "it always works as intended").

For subtyping, of course, we should still simply ignore the difference between all top types (I've been doing a lot of experiments with voidness preservation, and I do not think there is any subtype (micro) structure which will give us the desired outcome, i.e., voidness preservation cannot simply be expressed as "the usual subtype rules, just taking the micro structure into account").

My current stance is that we ought to try using a micro structure on subtyping (possibly "Object <: dynamic <: void") at least for specificity & interface computations, but we should be prepared to have completely separate treatment of these non-subtype type distinctions for some other purposes.

@kmillikin
Copy link

kmillikin commented Jan 15, 2018

45b02f8 fixes the type inference bug that was affecting package test.

Type inference will now infer a return type of void for operator []=, which is the issue affecting package yaml. That change to inference has been written in the spec but not yet implemented in the front end. See issue #31779.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end-fasta legacy-area-front-end Legacy: Use area-dart-model instead. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

7 participants