Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type named $ is replaced by empty string in error messages #15381

Closed
TomasMikula opened this issue Jun 5, 2022 · 17 comments
Closed

Type named $ is replaced by empty string in error messages #15381

TomasMikula opened this issue Jun 5, 2022 · 17 comments
Labels
itype:enhancement Spree Suitable for a future Spree

Comments

@TomasMikula
Copy link
Contributor

TomasMikula commented Jun 5, 2022

Compiler version

3.1.3-RC2

Minimized code

case class $[A](value: A)

def f(x: Int): Unit = {}

f(T(3))

Output

Found:   [Int]
Required: Int

Notice the missing $.

Expectation

Found:    $[Int]
Required: Int
@TomasMikula TomasMikula added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 5, 2022
@odersky odersky added Spree Suitable for a future Spree and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 6, 2022
@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jun 8, 2022

Names with $ are reserved for compiler internal use by the language specification, their usage in user code has undefined behavior. Therefore this is not a bug.

@nicolasstucki nicolasstucki added itype:enhancement Spree Suitable for a future Spree and removed itype:bug Spree Suitable for a future Spree labels Jun 8, 2022
@nicolasstucki
Copy link
Contributor

We could still try to print the $ anyway

@Florian3k
Copy link
Contributor

I've tried to debug this. Trailing $ are being removed by RefinedPrinter.nameString:

override def nameString(name: Name): String =
  def strippedName = if printDebug then name else name.stripModuleClassSuffix
  ...

I understand that this is desired behaviour, because we want do display compiler-generated names without trailing dollars. I don't see any easy way to distinguish between user provided names and those generated by compiler.

Potentially, we could add field to Name indicating whether it's coming from source code or not, but I suspect that this will require significant changes. Since using $ inside names is undefined behaviour, I don't think making such changes is desirable.

@TomasMikula
Copy link
Contributor Author

Why then allow $ in source names at all? There should be some consistency.

@som-snytt
Copy link
Contributor

stripModuleClassSuffix should not strip the bare $, a suffix which is also a prefix. Clearly an empty name is wrong.

$ is not forbidden because we all need to do some Java interop on occasion; also some Scala interop. One might refer to A$B in tooling.

@Florian3k
Copy link
Contributor

This problem exists with any name ending with $, example:

case class T$[A](value: A)

def f(x: Int): Unit = {}

@main def main = f(T$(3))

Output:

5 |@main def main = f(T$(3))
  |                   ^^^^^
  |                   Found:    T[Int]
  |                   Required: Int
  |
  | longer explanation available when compiling with `-explain`

@som-snytt
Copy link
Contributor

som-snytt commented Jul 5, 2022

There are perverse usages of $, which must run afoul of internal conventions, and possibly reasonable usages, such as a regex library that wants to name something $. Or a monetary unit called $.

It might be useful to add a FAQ about these edges.

@nicolasstucki
Copy link
Contributor

case class T$[A](value: A)

Will not compile correctly, the class T$ will conflict with the class of the companion object with the same name. One will override the other. This behavior is consistent with the undefined behavior of names using $.

@nicolasstucki
Copy link
Contributor

The best thing we could do is to identify names containing $ and emit a warning. Most users do not know that these names are reserved for compiler generated code.

@som-snytt
Copy link
Contributor

I don't know about the best thing.

This does the right thing for innocent $: #15597

I probably would not write object syntax { object $ }.

@jducoeur
Copy link
Contributor

jducoeur commented Jul 6, 2022 via email

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jul 7, 2022

The authors or maintainers of jQuery wrapper library have been told many years ago that their use of the $ can hit undefined behavior. The simple fix for them is to use `$` instead.

@som-snytt
Copy link
Contributor

A backquoted identifier is usually just an identifier. The famous exception is it's not a pattern varid. The recent exception is it's not a unary prefix operator. Backquotes also change syntax for leading infix operators: instead of an exclusion, it lets a non-symbolic identifier "opt in" as leading infix.

Backquotes don't have uniform semantics, but are more like variance: add backquotes until it compiles. If the language had adopted guillemets, there would be more quotes to try before giving up.

Of course, autocorrect just changed guillemets to guillemots, which is the bird even though mot means word. Worth acknowledging that the Mets baseball team is having a pretty good year.

@som-snytt
Copy link
Contributor

The issue about accessing Scala MODULE$ from Java makes me think messages should never strip dollar in a Java context.

7 |import bar.slf4j.ScalaMDCAdapter$;
  |                 ^
  |                 value ScalaMDCAdapter is not a member of bar.slf4j - did you mean slf4j.ScalaMDCAdapter?

#15608

@Billal-B
Copy link

Billal-B commented Nov 3, 2022

It appears to be working :

case class $[A](value: A)

def f(x: Int): Unit = {}

val x = f($(3))

Error output :

-- [E007] Type Mismatch Error: /home/billal/Projects/foss/dotty_playground/15381/15381.scala:5:11 
5 |val x = f($(3))
  |          ^^^^
  |          Found:    $[Int]
  |          Required: Int
  |
  | longer explanation available when compiling with `-explain`

See test added by the following commit : b5203de

@anatoliykmetyuk
Copy link
Contributor

Found and closed during the Scala spree at ScalaIO 2022 - thanks @Billal-B for spotting!

@som-snytt
Copy link
Contributor

My previous comment may have been why I thought it might not be closed #15381 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
itype:enhancement Spree Suitable for a future Spree
Projects
None yet
Development

No branches or pull requests

8 participants