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

Add support for treating generic method parameters as dynamic in all contexts #27460

Closed
leafpetersen opened this issue Sep 30, 2016 · 11 comments
Closed
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@leafpetersen
Copy link
Member

The current implementation of generic method syntax in the VM and dart2js treats generic type parameters as malformed types, which get mapped to dynamic in some (but not all) contexts. In particular, they are not usable in is or as checks. In order to remove the comment syntax fully from strong mode, we will need an option in the VM and dart2js to enable a mode in which all uses of generic method parameters are mapped to dynamic.

@leafpetersen leafpetersen added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. web-dart2js area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). labels Sep 30, 2016
@crelier
Copy link
Contributor

crelier commented Oct 1, 2016

I have sent a cl for review addressing this in the VM. I did not add another mode, and the new behavior is what you get with --generic-method-syntax.

@eernstg
Copy link
Member

eernstg commented Oct 3, 2016

Actually, I recommended a change to the vm implementation in #27437 such that it would treat a method type parameter as dynamic when performing checked mode checks. The rationale was that the intention behind type annotations is that the dynamic behavior of the program should satisfy the implied constraints. Hence, in a correct program there should never be a failure during such a check, and silently succeeding will not introduce bugs into correct programs.

An is check is different: It is perfectly plausible that a correct program uses an is check in such a way that it should evaluate to false in certain situations. This means that it would be misleading for an implementation to silently evaluate x is T to true in all cases, it would silently introduce logical bugs into correct programs. That's the reason why dart2js raises a runtime error on is and is! checks with method type parameters.

The as check falls between these two extremes. We could put it into any of the buckets ("it's just like a checked/strong mode check on a type annotation" vs. "it's a dynamic check, just like is"), and it will currently raise an error in dart2js.

In short, I'm not convinced that we should let x is T return true in all cases.

@eernstg
Copy link
Member

eernstg commented Oct 3, 2016

The CL https://codereview.chromium.org/2388843002/ implements a new semantics for o as T in dart2js: It is checked as if T had been dynamic. I haven't changed the semantics of o is T, based on the worries I described in the previous comment.

@eernstg
Copy link
Member

eernstg commented Oct 3, 2016

Note that we haven't actually settled what the semantics should be.

@floitschG
Copy link
Contributor

I'm not sure allowing is checks on generic method arguments is a good idea. It won't work in dart2js and in the VM and there isn't even a good default way of dealing with it. If it's really much easier to deal with in the VM, then we can/should go this route, but if it doesn't make a big difference, then I would rather discourage users from doing is/as checks on these arguments until the language actually handles them.

Independently reopening since only the VM would satisfy the original request (and this issue has also been filed for dart2js and the language).

@floitschG floitschG reopened this Oct 3, 2016
@leafpetersen
Copy link
Member Author

Yes, it's really not ideal that we will get different behavior on the VM/dart2js vs DDC for these checks, but unfortunately we're already in that state because of the comment syntax. I did some code searching, and I can't find any uses of the comment syntax for is checks (thankfully!). There are many uses of the comment syntax for as checks. So I think we could get away with the compromise position of treating uses of generic parameters in is checks as an error, but allowing them everywhere else.

We may want to consider issuing a static warning for is checks on generic method parameters for the time being as well (i.e. in the analyzer).

@floitschG floitschG removed web-dart2js area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. labels Oct 3, 2016
@floitschG
Copy link
Contributor

Removed area-dart2js and area-vm from the bug.
We shouldn't have bugs that cover multiple areas, and there is no reason to include them as long as we don't have an agreement in the language team.

Yes. I think is checks should not be allowed.
The as checks are probably to work around the type system and could be ignored. (Would be nicer to have Picard, since that's a better indication of "I don't actually want the check").

@leafpetersen
Copy link
Member Author

We discussed this in the language meeting this morning, and will follow up with clarification shortly. Sorry for the confusion.

@leafpetersen leafpetersen added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. and removed area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). labels Oct 20, 2016
@leafpetersen
Copy link
Member Author

We have an informal spec for this. I believe that the VM implementation is already compatible with this. @crelier can you please verify, and close this bug if so?

https://docs.google.com/document/d/1nccOSkdSyB9ls7TD8gUqIu6Wl2J_9bTSOoMCEIoAiQI/edit#heading=h.dnanv15dn1me

@crelier
Copy link
Contributor

crelier commented Oct 20, 2016

According to the revised informal spec, the errors resulting from using a function type parameter in some contexts are always static errors, and not compile time or run time errors. Therefore, the implementation in the VM is free to map a function type parameter to dynamic in all contexts, which it does today.
Closing this issue.

@crelier crelier closed this as completed Oct 20, 2016
@eernstg
Copy link
Member

eernstg commented Oct 21, 2016

Very good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

4 participants