support typechecker desugaring in conversion from Redhat AST

[jvasileff]

Desugaring performed by the typechecker results in AST that ceylon.ast.redhat can't handle (eclipse-archived/ceylon#4406 (comment)):

Exception in thread "main" ceylon.language.AssertionError "Assertion failed: Need CommonToken to get length of token (!= text’s length for \iCONSTANT)
    violated is CommonToken token = identifier.mainToken"
    at ceylon.ast.redhat.lIdentifierToCeylon_.lIdentifierToCeylon(Identifier.ceylon:51)
    at ceylon.ast.redhat.parameterReferenceToCeylon_.parameterReferenceToCeylon(ParameterReference.ceylon:16)
    at ceylon.ast.redhat.parameterToCeylon_.parameterToCeylon(Parameter.ceylon:52)
    at ceylon.ast.redhat.parametersToCeylon_$2.$call$(Parameters.ceylon)
    at ceylon.ast.redhat.parametersToCeylon_$2.$call$(Parameters.ceylon)
    at ceylon.ast.redhat.propagateUpdate_$1.$call$(propagateUpdate.ceylon:16)
    at ceylon.ast.redhat.propagateUpdate_$1.$call$(propagateUpdate.ceylon:16)

[jvasileff]

In more limited cases, like for Conditions, ceylon.ast.redhat "re-sugars" the tree. But, perhaps for these more aggressive de-sugarings, it would be easier/more expedient to just accept the modified tree?

The downside is that the resultant Ceylon AST would not represent the original program structure.

[lucaswerkmeister]

I think I’d prefer to resugar this as well. Or would the desugared version be significantly easier to implement for you?

[jvasileff]

In theory, the desugared version requires no work on my part (I think the JVM and JS backends got this feature "for free".)

Going forward, re-sugaring will result in triple work. So I'm not sure that's a great idea, especially if this is going to occur more often. OTOH, maybe it is important to be able to perform transformations, pretty-print, etc. when starting with a RH AST.

Perhaps @gavinking can weigh in on this.

[jvasileff]

Maybe it would be best to do both (if re-sugaring is desirable), and have re-sugaring be controlled by a flag.

[lucaswerkmeister]

Well we still have this problem with the _0 names. I feel like one viable solution might be to discard the desugaring approach in the backend and instead just make the backends support it, just like they support destructuring in loops as well (even though that too could be desugared).

[gavinking]

If it makes a difference: I see the current implementation as temporary. It would be much better if we eventually stopped desugaring this syntax and passed the original AST straight to the backends.

[jvasileff]

@lucaswerkmeister after thinking about this, I'd prefer the type checker's version of the tree, even if re-sugaring is done for other purposes.

It does, for some hard to explain reason, seem "cleaner" to go through re-sugaring. But it adds work and unnecessarily serializes development. Furthermore, I'd like to refactor the Dart compiler so it performs a large backend-neutral de-sugaring (post-typechecking) before code generation. Keeping the de-sugaring is, in a way, consistent with this goal.

[lucaswerkmeister]

Well if @gavinking plans to remove the current implementation, what should I do? Re-de-sugar and also mock up type information for the new variables?

[jvasileff]

Haha, no, I think at that point all the backends will have to deal with it.

But ceylon.ast.redhat would have to support patterns in that position, and I suppose has to now as well, since it may consume RH ASTs that haven't been type checked/de-surgared.

I guess the issue is that non-backend users of ceylon.ast may have different needs when using an ast that started as a type-checked typechecker ast. But I'm not sure what those use cases are.

[lucaswerkmeister]

This is now covered by #116 and #119, isn’t it?

[jvasileff]

@lucaswerkmeister if you're happy with the commits above, I think we're good on this one!

[lucaswerkmeister]

Alright, thanks.

[lucaswerkmeister]

Wait, which commits above? I think I’ve lost track. Are there some commits in your fork that need to move over into this one? #116 isn’t fixed, right?

[jvasileff]

Tokens are no longer required after f2c2b33 and bac9ff4, which I think covers the original error.

[lucaswerkmeister]

Oh I see, those PRs got merged. Then I guess we can close this now.