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

Update phrasing for NotClassType explain error message #19635

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

aherlihy
Copy link
Contributor

@aherlihy aherlihy commented Feb 6, 2024

Fixes #14175

@SethTisue
Copy link
Member

(I recorded our design thinking on the ticket.)

@aherlihy aherlihy force-pushed the i14175 branch 2 times, most recently from d55938e to e4a32e3 Compare February 6, 2024 17:39
@SethTisue
Copy link
Member

checkfile updates are needed in tests/neg-scalajs, CI says

@som-snytt
Copy link
Contributor

the other day, I was pondering the subtleties of trying to align the text

s"""abc
    |def""".stripMargin
sm"""|abc
     |def"""

ultimately the editor must autofill the margin char, which may or may not fall on a tabstop.

def explain(using Context) =
i"""A class type includes classes and traits in a specific order. Defining a class, even an anonymous class,
|requires specifying an order for the traits it extends. For example, `A & B` is not a class type because it
|doesn't specify which trait takes precedence, A or B. Both `A with B` and `B with A` are class types.
Copy link
Member

@smarter smarter Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Scala 3 you can also write class Foo extends A, B instead of extends A with B, it might be simpler to speak about a list of class types instead.

def explain(using Context) = ""
def explain(using Context) =
i"""A class type includes classes and traits in a specific order. Defining a class, even an anonymous class,
|requires specifying an order for the traits it extends. For example, `A & B` is not a class type because it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the message should mention "linearization order" since this is the key term to google to know what the precedence order actually specifies.

@SethTisue
Copy link
Member

SethTisue commented Feb 13, 2024

@smarter Thanks for taking a look. Re-review now?

Comment on lines 2779 to 2782
i"""A class type includes classes and traits in a specific order. Defining a class, even an anonymous class,
|requires specifying a linearization order for the traits it extends. For example, `A & B` is not a class type
|because it doesn't specify which trait takes precedence, A or B. Both the list `A,B` and `B,A` are class types.
|Class types also can't have refinements."""
Copy link
Member

@smarter smarter Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be overly pedantic since it seems a lot of time has been spent on this already, but a list of class types isn't a class type itself. If we take a concrete example like:

Suggested change
i"""A class type includes classes and traits in a specific order. Defining a class, even an anonymous class,
|requires specifying a linearization order for the traits it extends. For example, `A & B` is not a class type
|because it doesn't specify which trait takes precedence, A or B. Both the list `A,B` and `B,A` are class types.
|Class types also can't have refinements."""
scala> class A extends Object, (Serializable & Product)
-- [E170] Type Error: ----------------------------------------------------------
1 |class A extends Object, (Serializable & Product)
| ^^^^^^^^^^^^^^^^^^^^^^^^
| Serializable & Product is not a class type

Then we want to replace the type Serializable & Product by a list of types Serializable, Product.

Another issue here is that the comma syntax does not work with anonymous classes, new (Serializable & Product) needs to be rewritten as new Serializable with Product

Instead of giving formal definitions, it might be clearer to just give a couple of examples to the user, and refer to the spec for further details:

Suggested change
i"""A class type includes classes and traits in a specific order. Defining a class, even an anonymous class,
|requires specifying a linearization order for the traits it extends. For example, `A & B` is not a class type
|because it doesn't specify which trait takes precedence, A or B. Both the list `A,B` and `B,A` are class types.
|Class types also can't have refinements."""
i"""A list of parents can only include classes or traits, to inherit multiple traits write:
| class A extends B, C
|or for an anonymous class:
| new B with C
|Note that the order in which parent traits are inherited influences overriding resolution,
|for more information, refer to "Class Linearization" in:
|<https://www.scala-lang.org/files/archive/spec/${config.SourceVersion.defaultSourceVersion}/05-classes-and-objects.html>""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion on the ticket to add:

Did you mean `Serializable, Product`?

or

Did you mean `Serializable with Product`?

as context dictates. I would offer that without -explain. Another view of messaging that comes up in this repo is that the error is a prompt for correction, and usually the user either knows right away what to do or needs a quick reminder only.

I agree that if the explain does not have diagnostic info, then some examples for the "gist" of it is most helpful, rather than an exhaustive definition of terms.

I don't know if linking to the spec is more explanatory than the general docs, though the URL will be more stable.

Ask your team lead what they think a class type is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if linking to the spec is more explanatory than the general docs

Is there a general docs page that talks about linearization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have multiple contexts in which this error could be thrown, it would only confuse users more to see explanations like class A extends Object, (Serializable & Product) if this error is being thrown elsewhere. Because there's no easy fix, I suggest providing a simple example without a suggestion that may not work depending on the context (e.g. A with B may not work everywhere). I will make an issue in the documentation repo to add a page about class types so that users don't have to read the language spec, but in the meantime add a reference to the specification in the error message so users can read up about class types.

@som-snytt
Copy link
Contributor

Sample Scala 2 message where I wish we had "clik-thru" data to have some idea of how many customers actually investigated.

       Note: Any >: Char, but class Array is invariant in type T.
       You may wish to investigate a wildcard type such as `_ >: Char`. (SLS 3.2.10)

There is a hazard of directing users down an alley where they find the way barred and locked from the outside. All they want is to fix the code.

Either the message is a brief prompt that reminds the user of that which they know so well (IIRC in one of the movies, Spock tells Kirk, "I would not remind you of that which you know so well." I feel like I'm omitting a preposition.) or give them an affiliate link for the Odersky book.

Oh that was an easy search: https://memory-alpha.fandom.com/wiki/Vulcan_philosophy which reminds us that Vulcan philosophy is based upon smug self-assurance.

We re-watch The Wizard of Oz to be reminded of that witch we know so well.

@smarter
Copy link
Member

smarter commented Feb 15, 2024

@aherlihy made me realize that the NotClassType message is used in more situations than I thought, for example:

trait A
trait B
val x = classOf[A & B] // not a class type

In that kind of situation, neither "A with B" nor "A, B" will help, and linearization isn't relevant.

Maybe keeping the message short and deferring to docs/specs is best at that point. We could start specializing it depending on the use-site but I'd leave that for future work.

@aherlihy
Copy link
Contributor Author

See scala/docs.scala-lang#2975, feel free to add any suggestions for clearer documentation.

@SethTisue
Copy link
Member

squash/rebased to two commits — let's merge once CI likes it again

@SethTisue SethTisue enabled auto-merge February 19, 2024 00:28
@SethTisue SethTisue merged commit 9a79c14 into scala:main Feb 19, 2024
17 checks passed
@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
WojciechMazur added a commit that referenced this pull request Jul 2, 2024
…LTS (#20923)

Backports #19635 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"is not a class type" error for given for intersection type
5 participants