Trees in statement position may have incorrect types #4442

gzm0 · 2021-02-26T12:17:22Z

gzm0 · 2021-02-26T12:54:51Z

There seem to be two independent issues:

When transforming If, we do not set the proper result type based on isStat:

https://github.com/scala-js/scala-js/blob/master/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala#L2314-L2316

Other transformations have this pattern:

val tpe =
  if (isState) jstpe.NoType
  else toIRType(tree.tpe)

Adding this fixes some of the IR checking errors.

The other issue is in JS constructor export generation: The export machinery post transforms the trees generated by genExportMethod. In that process, it moves trees that were meant for expression position (overload dispatch and forwarder calling) into statement position. While it adjusts the inner trees, it doesn't adjust the types of the surrounding trees.

I have not yet been able to fix this problem (it seems that post-transforming trees is unwise, an approach that simply generates the right trees upfront seems better).

gzm0 · 2021-03-14T16:45:44Z

An update on this: I'm currently working on improving how JS exporter generation works. This takes quite some time unfortunately.

Instead of post-transforming the trees generated by GenJSExport, we re-factor the right parts and generated the proper trees right away. This was neccessary to give the generated trees the right type (partial scala-js#4442).

…osition In statement position, they must always be typed as NoType, since their children might be typed as such. We condition the deserialization hack introduced in the parent commit so that it only applies to old code.

Instead of post-transforming the trees generated by JSExportsGen, we refactor the right parts and generate the proper trees right away. This was neccessary to give the generated trees the right type (partial scala-js/scala-js#4442). Forward port of scala-js/scala-js@8093f28

Recent changes have all but removed the distinction between statements and expressions in the IR. Notably: * bb4f6da Allow Return arg to be a void if the target Labeled is a void. * cdcff99 Rename NoType to VoidType and print it as "void". Therefore, it made no sense anymore that `Transformer.transform` had an `isStat` argument. In fact, the first of the two commits mentionned above semi-broke the semantics of that argument anyway. Moreover, `isStat` was never *used* in our codebase (only forwarded everywhere for no reason). The only exception was a deserialization hack for IR <= 1.5. This one was already hacking around the fact the IR produced back then was not well-typed. See scala-js#4442 for context. We now remove that parameter everywhere. It makes the deserialization hack a bit more verbose, but everything else becomes simpler.

gzm0 added the bug Confirmed bug. Needs to be fixed. label Feb 26, 2021

gzm0 self-assigned this Feb 26, 2021

This was referenced Apr 5, 2021

Replace topoSortDistinctsBy with topoSortDistinctsWith #4462

Merged

Remove dead code for anon JS class constructors #4464

Merged

gzm0 mentioned this issue May 25, 2021

Rework JS class constructor generation #4490

Merged

gzm0 added this to the v1.6.0 milestone May 30, 2021

gzm0 linked a pull request Jun 5, 2021 that will close this issue

Base stat/expr checking on the type system in IRChecker #4439

Merged

sjrd closed this as completed in #4439 Jun 7, 2021

sjrd mentioned this issue Dec 14, 2024

Remove the stat/expr distinction from Transformers.Transformer. #5087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Trees in statement position may have incorrect types #4442

Trees in statement position may have incorrect types #4442

Uh oh!

Uh oh!

Trees in statement position may have incorrect types #4442

Trees in statement position may have incorrect types #4442

Comments

Uh oh!

Uh oh!