SI-8425 don't create double-dollar names in c.freshName #3638
Conversation
If we append a dollar to a user-provided prefix that ends in a dollar, we create a potential for confusion for backend phases. That's why this commit prevents such situations from happening.
|
review @retronym |
|
LGTM but let's leave the merge decision up to @adriaanm |
|
Ok, I'll include this in RC2. What could possibly go wrong :) |
| import scala.language.experimental.macros | ||
| import scala.reflect.macros.blackbox.Context | ||
|
|
||
| object Macros { |
There was a problem hiding this comment.
A comment not blocking this PR: I think for tests like that we should have a lightweight way of instantiating macro context and turn this test into a unit test. It already is a unit test just using framework (partest) for functional tests.
The benefit is as usual: instead of having tests that takes 5s to run we would have test that takes 50ms to run and are easier to debug in the IDE.
There was a problem hiding this comment.
For inspiration see: https://github.com/scala/scala/blob/master/test/junit/scala/tools/nsc/symtab/SymbolTableForUnitTesting.scala
https://github.com/scala/scala/blob/master/test/junit/scala/tools/nsc/symtab/StdNamesTest.scala
It would be great to have "MacroContextForUnitTesting".
There was a problem hiding this comment.
Macros very often require a fully-operational typechecker. Is this something that has already been attempted in unit tests?
There was a problem hiding this comment.
I'm not aware of such attempt. However, I was thinking strictly about tests that do not require type checker like the one presented here. I'm not sure what's the portion of tests that fall into the category of easily unit testable.
SI-8425 don't create double-dollar names in c.freshName
If we append a dollar to a user-provided prefix that ends in a dollar,
we create a potential for confusion for backend phases. That's why
this commit prevents such situations from happening.