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

Missing error messages #1589

Closed
felixmulder opened this issue Oct 13, 2016 · 120 comments · Fixed by #8885
Closed

Missing error messages #1589

felixmulder opened this issue Oct 13, 2016 · 120 comments · Fixed by #8885
Labels
area:documentation area:reporting Error reporting including formatting, implicit suggestions, etc exp:novice help wanted itype:enhancement

Comments

@felixmulder
Copy link
Contributor

felixmulder commented Oct 13, 2016

We have a multitude of errors that need to be ported to the new scheme - you'll find these under "Error Messages" below. But first, a tutorial:

HOWTO on creating new messages

Once you've identified an error you want to replace, you should decide what semantic information is needed in the message - i.e. the position, perhaps a tree or some types. For this example we're going to be implementing the error message for the following error:

case class Foo // you're not allowed to have a case class without a single parameter list!

So we create a file with this and compile it using ./bin/dotc test.scala. This gives us the error message:

case class Foo
           ^
           case class needs to have at least one parameter list

So now we search the repository for "case class needs to have" - I do this with git grep -n "case class needs to have".

And now we find that in src/dotty/tools/dotc/ast/Desugar.scala we have the following line:

ctx.error("case class needs to have at least one parameter list", cdef.pos)

Cool - now we know what to replace. So next step is to add a case class to src/dotty/tools/dotc/reporting/diagnostic/messages.scala. The case class that we add should extend Message:

abstract class Message(val errorId: Int) {
  /** The `msg` contains the diagnostic message e.g:
    *
    * > expected: String
    * > found:    Int
    *
    * This message wil be placed underneath the position given by the enclosing
    * `MessageContainer`
    */
  def msg: String

  /** The kind of the error message is something like "Syntax" or "Type
    * Mismatch"
    */
  def kind: String

  /** The explanation should provide a detailed description of why the error
    * occurred and use examples from the user's own code to illustrate how to
    * avoid these errors.
    */
  def explanation: String
  ...
}

Now we name our case class something useful and give it a unique errorId:

case class CaseClassMissingParamList() extends Message(4)

Since we can see that the call to ctx.error took the position of a cdef: TypeDef, we could perhaps use this in the constructor of our Message?

case class CaseClassMissingParamList(cdef: untpd.TypeDef) extends Message(4)

It might also be that we're going to be manipulating cdef tree, as such we should pass a Context to get all the implicit operations:

case class CaseClassMissingParamList(cdef: untpd.TypeDef)(implicit ctx: Context)
extends Message(4)

Alright! Now we can start adding things to our Messsage:

case class CaseClassMissingParamList(cdef: untpd.TypeDef)(implicit ctx: Context)
extends Message(4) {
  val kind = "Syntax" // this will be shown as a delimiter: "-- [E004] Syntax X -------"

  // we can use the "hl" interpolator to give syntax highlighting to our message:
  val msg =
    hl"""|A ${"case class"} must have at least one parameter list"""

  // not everything that is interpolated will be highlighted, for instance you can use
  // colors like: Red("must") - this will get the red color.
  val explanation =
    hl"""|${cdef.name} ${Red("must")} have at least one parameter list, if you would rather
         |have a singleton representation of ${cdef.name}, use a "${"case object"}".
         |Or, add an explicit `()' as a parameter list to ${cdef.name}.""".stripMargin
}

Stable Identifier

So far, we've simplified the above cases a smidge, the error messages themselves actually don't extend Message by passing an integer. They should actually create an entry into the java enum defined in ErrorMessageID.java. This will enable us to have stable identifiers which helps tooling between the different compilers.

Now you replace the call to ctx.error(String) with ctx.error(CaseClassMissingParamList(cdef)) - and you're done with the implementation!

Tests

To test your solution, add a test to ErrorMessagesTests.scala. An example for TypeMismatch is provided and documented.

Online documentation

We really want to have online documentation providing more detailed information on what went wrong. Currently the idea is to create markdown pages in the docs/docs folder. Any contribution here would be greatly appreciated!

Things to keep in mind

  • You're creating a semantic object - this object will either be the output of the terminal or it could be used in an IDE, so make sure to include the relevant things. Typically: Types, Position, Tree
  • Name your error something easy to understand!
  • If you think your error doesn't need an explanation - that's fine - just put val explanation = ""
  • If you find yourself not being able to manipulate trees - do you have the implicit context?
  • "HALP!" - please post in our gitter channel: https://gitter.im/lampepfl/dotty or comment on this issue 😃

Missing Messages

Please comment below saying which messages you'd like to tackle.

Available

  • Applications.scala:95 (@aesteve)
  • Checking.scala:49
  • Checking.scala:526 (@benkobalog)
  • Checking.scala:533
  • Checking.scala:539
  • Checking.scala:555
  • Checking.scala:565
  • Checking.scala:576
  • Checking.scala:583
  • Checking.scala:602
  • Checking.scala:622
  • Checking.scala:628
  • Checking.scala:646
  • Checking.scala:648
  • Checking.scala:692
  • Checking.scala:702
  • Checking.scala:716
  • Docstrings.scala:30
  • Namer.scala:222
  • Namer.scala:258
  • Namer.scala:1081
  • Namer.scala:1105
  • Parsers.scala:2266
  • Parsers.scala:2438
  • Parsers.scala:2456
  • PatternMatcher.scala:911
  • RefChecks.scala:694
  • RefChecks.scala:698
  • RefChecks.scala:874
  • SymDenotations.scala:1964
  • TypeAssigner.scala:34
  • TypeAssigner.scala:208
  • TypeAssigner.scala:233
  • TypeAssigner.scala:299
  • TypeAssigner.scala:314
  • TypeAssigner.scala:356
  • TypeAssigner.scala:358
  • TypeAssigner.scala:416
  • Typer.scala:174
  • Typer.scala:196
  • Typer.scala:355
  • Typer.scala:411
  • Typer.scala:866
  • Typer.scala:1172 (@Wojtechnology)
  • Typer.scala:1519
  • Typer.scala:1572 (on hold)
  • Typer.scala:1731
  • Typer.scala:1924
  • Typer.scala:2015

Contributors

@felixmulder felixmulder added itype:enhancement help wanted area:reporting Error reporting including formatting, implicit suggestions, etc area:documentation labels Oct 13, 2016
@markusthoemmes
Copy link

markusthoemmes commented Oct 14, 2016

This is an awesome way of empowering people to contribute to the project. I think I'll use this to contribute. Thanks a lot for writing this up @felixmulder!

@markusthoemmes
Copy link

To avoid duplicated work: I'm taking on Parsers.scala:403 (shall we just edit the issue description and put names behind the specific messages to keep it transparent for everyone?)

@felixmulder
Copy link
Contributor Author

felixmulder commented Oct 15, 2016

@markusthoemmes: yes! Done :) Added a clarification to the top :)

@anicolaspp
Copy link

👍

@sebastianharko
Copy link

sebastianharko commented Oct 15, 2016

Indeed, this is an awesome way to get people to contribute to the project ... I'm tackling Parsers.scala:2064

@ShaneDelmore
Copy link
Contributor

I'll take Desugar.scala and ErrorReporting.scala.

@sebastianharko
Copy link

sebastianharko commented Oct 16, 2016

@felixmulder: I created a PR for Parsers.scala: 2064 :-)

@ljdelight
Copy link
Contributor

Hi, I'm new here. I'll take Parsers.scala:1737, Parsers.scala:1738, Parsers.scala:1739, Parsers.scala:1901

@sebastianharko
Copy link

sebastianharko commented Oct 16, 2016

Taking Parsers:1492, Parsers:1329, Parsers: 626 and TypeAssigner.scala:449

@AndrewZurn
Copy link
Contributor

Was just sitting down with @ShaneDelmore and looking at these a little bit. Looks like quite a few Parsers and TypeAssigner messages left, if there are one or two 'easier' messages in there that I can take a look at, would someone mind assigning those to me?

Andrew

@jyotman
Copy link

jyotman commented Oct 18, 2016

Hi, Working on Parsers.scala:1180.

@felixmulder
Copy link
Contributor Author

@AndrewZurn - how about Parsers.scala:695 to start off with? I'll assign you, but if you don't want it let me know :)

@sebastianharko
Copy link

sebastianharko commented Oct 22, 2016

@felixmulder can you mark Parsers:626 and Parsers:1492 as complete :-) ?

@felixmulder
Copy link
Contributor Author

@sebastianharko - yes done! Thanks

@iamthiago
Copy link
Contributor

iamthiago commented Oct 23, 2016

Hi, also new over here, just trying to help this lovely project... I will work for now on Comments.scala:128, JavaParsers.scala:233 and Parsers.scala:319 ok?

@felixmulder
Copy link
Contributor Author

@thiagoandrade6 - sounds great :)

iamthiago pushed a commit to iamthiago/dotty that referenced this issue Oct 23, 2016
This commit adds the semantic object for the ```definition not found``` error.
It is part of the (scala#1589
iamthiago pushed a commit to iamthiago/dotty that referenced this issue Oct 23, 2016
This commit adds the semantic object for the ```definition not found``` error.
It is part of the (scala#1589
iamthiago pushed a commit to iamthiago/dotty that referenced this issue Oct 24, 2016
This commit adds the semantic object for the ```definition not found``` error.
It is part of the (scala#1589
iamthiago pushed a commit to iamthiago/dotty that referenced this issue Oct 24, 2016
This commit adds the semantic object for the ```definition not found``` error.
It is part of the (scala#1589
iamthiago pushed a commit to iamthiago/dotty that referenced this issue Oct 25, 2016
This commit adds the semantic object for the ```definition not found``` error.
It is part of the (scala#1589
iamthiago added a commit to iamthiago/dotty that referenced this issue Oct 25, 2016
@evgeniiloikov
Copy link

Typer.scala:866 should be done here #7385

@harpresing
Copy link
Contributor

Working on RefChecks.scala:119

@x3ro
Copy link
Contributor

x3ro commented Oct 27, 2019

Namer.scala:258 is tackled by #7457

x3ro added a commit to x3ro/dotty that referenced this issue Oct 28, 2019
x3ro added a commit to x3ro/dotty that referenced this issue Oct 28, 2019
Part of the effort documented in scala#1589 to port all error messages to
the new scheme.
@scala scala deleted a comment from peykaz Nov 27, 2019
@hope-doe
Copy link

Hi!
Desugar.scala:457 done there #7729

@hope-doe
Copy link

Hi!
Parser:1426, 1428 done there #7848
(and #7729 is fixed also)

@jvdp
Copy link
Contributor

jvdp commented Jan 12, 2020

Starting from the top I did Checking.scala:49 53 and I've made a PR for it.

@anatoliykmetyuk
Copy link
Contributor

The architecture for errors described in this issue proved to be heavy and impractical. The migration process frequently interferes with in-flight PRs and the benefits are marginal. We should find a more light-weight way to represent errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation area:reporting Error reporting including formatting, implicit suggestions, etc exp:novice help wanted itype:enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.