fixes #15, strong mode errors

[jmesserly]

most of it was refactoring to avoid an assignment in a closure, which disabled type promotion

[jmesserly]

CC @nex3 @devoncarew

by the way, this doesn't address warning (many of which would cause runtime failures due to invalid generic casts) -- working on that in a follow up

[jmesserly]

I went ahead and fixed warnings too. Note that the .map is an issue with a particular kind of inference that's missing (parameters to a function expression): dart-archive/dev_compiler#203

[nex3]

most of it was refactoring to avoid an assignment in a closure, which disabled type promotion

This makes the code a lot harder to read—the point of those closures was to make "A then maybe B or B then maybe A" read as clearly as that sentence, and adding extra boilerplate hurts that. It's also not clear as a user that these particular assignments would causing problems, or why. Is this something that will be fixed eventually? Is there a bug filed? Should there be?

Note that the .map is an issue with a particular kind of inference that's missing (parameters to a function expression): dart-archive/dev_compiler#203

I'd like to hold off on adding workarounds for bogus warnings that will be fixed.

[jmesserly]

Is this something that will be fixed eventually? Is there a bug filed? Should there be?

No, I don't think this one is fixable. The method is too complex for type promotion to work well there (indeed, it didn't work before--var just disabled all messages--the warnings now come because we have local type inference).

I'm happy to try another way of refactoring that method. A few options that came to mind:

• insert as checks where necessary
• capture token into a new local variable before each if, then the variable isn't assigned and can be type promoted. If we had pattern matching in Dart, it would work more like that.
• split the method after parseAnchor/parseTag. In the new method, token wouldn't be reassigned.

Let me know which one you prefer.

I'd like to hold off on adding workarounds for bogus warnings that will be fixed.

that's why I CC'd @devoncarew. I think he needs this much sooner than we will have a fix for that. Sometimes it takes a small step back to take a bigger step forward :). Happy to come back and fix this code once we have a fix for that one.

[jmesserly]

Oh, another idea: we can mark token as dynamic. That disables all errors and warnings, at the cost of type checking & performance. That's how the old code was working.

[nex3]

Let me know which one you prefer.

Can we just declare token as a Token and avoid inference entirely?

that's why I CC'd @devoncarew. I think he needs this much sooner than we will have a fix for that. Sometimes it takes a small step back to take a bigger step forward :). Happy to come back and fix this code once we have a fix for that one.

Why do we need code to be DDC-warning-clean? That seems premature when there are still so many issues with the inference, and particularly when there's no way to write generic methods.

[sethladd]

Why do we need code to be DDC-warning-clean?

In this case, we want to use DDC for the Dart plugin for Atom. The Dart plugin for Atom needs to read YAML files.

[nex3]

DDC can compile without being warning-clean, right?

[devoncarew]

DDC can compile without being warning-clean, right?

I'd love a way to know which ddc warnings will impact me in different ways. Like:

• your code can't compile
• your code will probably break at runtime
• your code might have unintended runtime behavior
• the emitted code won't be as pretty as it could have been

The last item might be more important for some packages, less for others, and would be things that I'd probably work on slowly over time.

[jmesserly]

Can we just declare token as a Token and avoid inference entirely?

That's exactly what inference is concluding. You'll get the same errors. The problem with that method is it's assigning to token in a closure, which means "type promotion" can't figure out what is going on. DDC here is just exposing an issue that was already there--the code relies on dynamic dispatch. That's why dynamic token = ... works and Token token = ... doesn't.

(If Dart had pattern matching or something, instead of type promotion, we'd be in a much better place for code like that. But syntax would be different, so that would be an even bigger change to the method.)

[vsmenon]

@nex3 I don't see any issue in @jmesserly 's patch here that will be improved in DDC other than documents.map - at least nothing planned. For token, the simplest alternative fix is to type token as dynamic. That matches what non-strong mode is "inferring" today, but being explicit about it instead of silent. Typing as Token should given you warnings today without strong mode.

@devoncarew Errors are most serious. If you --force-compile, we'll generate code anyway and it might work, but bugs will be harder to catch. Warnings don't stop us from compiling - and if there really is a problem, we should throw a meaningful error at runtime - we'll generating a runtime check. Prettiness of output is useful due to dynamic operations - those are reported as just info right now.

[jmesserly]

Yeah, I'm happy to use dynamic token instead. Sorry about doing larger-than-necessary changes there.

[jmesserly]

I switched to dynamic token ... the CL is now down to 6 lines. Happy to squash changes too. For review purposes I left the history.

[nex3]

Please add a TODO to get rid of this.

[nex3]

I'd still like a better understanding of why the parser.dart change is necessary, and in particular why we can't annotate it as Token. What exactly goes wrong in the typechecker? It seems to me that if it's annotated as Token and all of the assignments to it have left-hand sides that are straightforwardly typed as Token, everything should be hunky-dory.

[vsmenon]

@nex3 If you explicitly type it as Token, then you get the following warnings even without strong mode:

[warning] The getter 'name' is not defined for the class 'Token' (/Users/vsm/git/yaml/lib/src/parser.dart, line 257, col 47)
[warning] The getter 'name' is not defined for the class 'Token' (/Users/vsm/git/yaml/lib/src/parser.dart, line 264, col 22)
[warning] The getter 'style' is not defined for the class 'Token' (/Users/vsm/git/yaml/lib/src/parser.dart, line 306, col 32)
[warning] The getter 'value' is not defined for the class 'Token' (/Users/vsm/git/yaml/lib/src/parser.dart, line 311, col 42)
[warning] The getter 'style' is not defined for the class 'Token' (/Users/vsm/git/yaml/lib/src/parser.dart, line 311, col 55)

[nex3]

Okay, so as I understand it: the issue is that the closure means that DDC can't prove that token's type doesn't change between an is check and a usage.

@jmesserly I have an alternate idea for getting rid of these warnings. If you remove the dynamic change, I'll fix them in a separate CL.

[jmesserly]

@jmesserly I have an alternate idea for getting rid of these warnings. If you remove the dynamic change, I'll fix them in a separate CL.

yeah, there's lots of possibilities. Go for it. I can rebase after your change lands.

[jmesserly]

Okay, so as I understand it: the issue is that the closure means that DDC can't prove that token's type doesn't change between an is check and a usage.

just to be clear, it's not coming from DDC. This is the Dart language spec & existing Dart Analyzer.

[nex3]

just to be clear, it's not coming from DDC. This is the Dart language spec & existing Dart Analyzer.

Okay, but the existing language spec never produces warnings here anyway.

[jmesserly]

yeah, that's a side effect of local type inference, which seems pretty non-controversial on its own. It can definitely introduce new errors, but only because previously Dart was suppressing all checks on var. (if we had to forever not have local type inference, it's likely most teams & style guides would go for Java-style local variable annotations. IMHO that'd be a much worse result.)

[jmesserly]

edit: I meant, "not controversial" ;)

[nex3]

Don't get me wrong, I'm definitely in favor of local type inference.

[nex3]

fb4313d fixes the parser warning.

[jmesserly]

PTAL, rebased against master, dropped dynamic token, added TODO for dart-archive/dev_compiler#203

[nex3]

👍

Can you bump the pubspec to a stable version before you merge?

[jmesserly]

Can you bump the pubspec to a stable version before you merge?

Done, PTAL

[jmesserly]

Thanks Natalie!