scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

sanity checks for macro expansions #8066

Closed scabug closed 6 years ago

scabug commented 10 years ago

There are some macro-induced compiler crashes in SuperAccessors, LambdaLift and GenICode (some of those documented at https://github.com/sbt/sbt/commit/3b213e59caafdc3b28bfbaf124e80a4584caf3aa) that we can easily avoid by providing a sanity-checking traverser for macro expansions. I will get to that this week.

scabug commented 10 years ago

Imported From: https://issues.scala-lang.org/browse/SI-8066?orig=1 Reporter: @xeno-by Affected Versions: 2.11.0-M7 See #5797, #7920, #7829, #7516

scabug commented 10 years ago

@xeno-by said: Problems that we'd like to prevent: 1) Untyped trees nested inside types trees leading to trees with null tpe escaping typer (crash in SuperAccessors) 2) Owner chain corruption (crashes in LambdaLift, GenICode): https://github.com/sbt/sbt/commit/3b213e59caafdc3b28bfbaf124e80a4584caf3aa#commitcomment-4735585 3) Sharing of trees between different parts of the expansion (now that's not a problem per se, because expansions are duplicated anyway, but it might screw people up when they typecheck a shared tree and another part of the expansion unexpectedly gets typed).

So #1 is definitely a compilation error.

2 looks like a compilation error as well, but we should actually consider detecting attributed trees spliced under symbol-introducing trees in the expansion and report that as as an error, because someone might end up splicing symbol-introducing trees there, even if in the current expansion it's just something simple. In 2.10.5 that should probably be an on-by-default warning that can be turned off by a -Y flag.

I think we should turn #3 into a compilation error as well. Even though it's going to work just fine, it's a symptom of something going wrong.

Finally, for better error reporting I think we could have quasiquotes remember their origins (maybe in attachments), so that we could say something like: "attempt to insert an untyped tree foo (defined in Foo.scala:42) under a type tree bar (coming from an argument of the macro expansion)".

scabug commented 10 years ago

@xeno-by said: Also see #5755

scabug commented 10 years ago

@xeno-by said: There's a really funny effect that happens when someone tries to return a statement (e.g. a MemberDef) from a macro impl. Then the macro engine says:

Test.scala:8: error: type mismatch;
 found   : <notype>
 required: Any
Note that <none> extends Any, not AnyRef.
Such types can participate in value classes, but instances
cannot appear in singleton types or in reference comparisons.
  Macros.foo
         ^
one error found

This should be caught by the validator. Also, it would be useful if the validator could tell people that such code doesn't make sense anyway, because the expanded definition is going to be local.

scabug commented 10 years ago

@xeno-by said: Another fun thing that's not directly related to this bug, but still I'll post it here to fix it together with the rest of the stuff. It looks like providing c.Expr[_] as a return type of a macro impl leads to funny effects, so we should probably disallow that, especially since we now allow c.Tree as the return type.

scabug commented 10 years ago

@xeno-by said: Somewhat related: https://github.com/scala/scala/pull/3326#issuecomment-31820859

scabug commented 10 years ago

@xeno-by said: Something to keep in mind given our recent untypecheck idea (https://groups.google.com/forum/#!topic/scala-internals/TtCTPlj_qcQ): #7516 + https://github.com/scala/scala/commit/83c9c764b5

scabug commented 10 years ago

@xeno-by said: Even though we don't have time to implement the untypecheck idea by RC1, we could still write a validator that warns users about potential owner chain corruptions and other problems and links them to the docs that explain the situation in more detail.

scabug commented 10 years ago

@xeno-by said: Related: #7920

SethTisue commented 6 years ago

closing since this never happened and it's far from clear that a change like this is likely ever to happen in the current macro system