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.

Sign up for GitHub

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

Jump to bottom

Fix #13320: add .type to modules in messages #13374

Merged
merged 2 commits into from Sep 14, 2021
Merged

Fix #13320: add .type to modules in messages #13374

merged 2 commits into from Sep 14, 2021

Conversation

bjornregnell
Copy link
Contributor

co-authored by: Anatolii Kmetiuk

@bjornregnell bjornregnell changed the title fix #13320 add .type to modules in messages Fix #13320: add .type to modules in messages Aug 24, 2021
@SethTisue
Copy link
Member

test coverage?

@bjornregnell
Copy link
Contributor Author

bjornregnell commented Aug 24, 2021 •
edited

I can try to write a test case for it.
With this input to the compiler:

object Foo:
  case object Boo

var x: Foo.Booo = Foo.Booo

object Main:
  def main(args: Array[String]) =
    type t = Option[Foo.Boo]

Above should with the fix now give these errors when scalac:

-- [E008] Not Found Error: /tmp/spree/test.scala:8:24 --------------------------
8 |    type t = Option[Foo.Boo]
  |                    ^^^^^^^
  |       type Boo is not a member of object Foo - did you mean Foo.Boo.type?
-- [E008] Not Found Error: /tmp/spree/test.scala:4:11 --------------------------
4 |var x: Foo.Booo = Foo.Booo
  |       ^^^^^^^^
  |      type Booo is not a member of object Foo - did you mean Foo.Boo.type?
-- [E008] Not Found Error: /tmp/spree/test.scala:4:22 --------------------------
4 |var x: Foo.Booo = Foo.Booo
  |                  ^^^^^^^^
  |          value Booo is not a member of object Foo - did you mean Foo.Boo?

Would that be an ok test case? Where to add it and how to name it?

@griggt
Copy link
Collaborator

griggt commented Aug 24, 2021 •
edited

I think the test should go in tests/neg/i13320.scala and should have an associated i13320.check file in the same directory to check the error messages.

Each line of the .scala file that is expected to generate a compiler error should be annotated with // error

More info on testing with checkfiles may be found here: https://dotty.epfl.ch/docs/contributing/testing.html#testing-with-checkfiles

@bjornregnell
Copy link
Contributor Author

bjornregnell commented Aug 24, 2021 •
edited

Thanks @griggt (I'll check this out in some week or so when I have gotten my course going...)

@griggt griggt linked an issue Aug 24, 2021 that may be closed by this pull request
Improve "did you mean" message when meaning to use object singleton type #13320
Closed
prolativ
prolativ reviewed Aug 25, 2021
View reviewed changes
compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated
@@ -300,14 +300,14 @@ import transform.SymUtils._

// The names of all non-synthetic, non-private members of `site`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The names of all non-synthetic, non-private members of `site`
// The symbols of all non-synthetic, non-private members of `site`

compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated
@@ -325,11 +325,11 @@ import transform.SymUtils._

// A list of possible candidate strings with their Levenstein distances
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// A list of possible candidate strings with their Levenstein distances
// A list of possible candidate symbols with their Levenstein distances

compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated
.map(n => (distance(n, missing), n))
.filter((d, n) => d <= maxDist && d < missing.length && d < n.length)
.sorted // sort by distance first, alphabetically second
.map(n => (distance(n.name.show, missing), n))
Copy link
Contributor

Choose a reason for hiding this comment

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

For less confusion I would advise to rename n to s all around here as we're now dealing with symbols instead of names

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use sym here (as in val candidates = ...) !

compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated
s" - did you mean $siteName.$n?$enumClause"
val showName =
// Add .type to the name if it is a module
if n.isClass && n.is(Module) then s"${n.name.show}.type"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you should get the same result with .is(ModuleClass) instead of checking two conditions

@bjornregnell
Copy link
Contributor Author

Great comments! Thanks @prolativ @michelou !
(I'll have a go as soon as I can find some spare time for this.)

@bjornregnell
fix scala#13320 add .type to modules in messages
4073d24 
co-authored by:
Anatolii Kmetiuk
anatoliykmetyuk@gmail.com
@bjornregnell @prolativ
add test, fix review comments
5400724 
Co-authored-by: Michał Pałka <prolativ@users.noreply.github.com>
@prolativ prolativ merged commit a0875e2 into scala:master Sep 14, 2021
@SethTisue
Copy link
Member

🎉

@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michelou michelou michelou left review comments

@prolativ prolativ prolativ approved these changes

Assignees

@bjornregnell bjornregnell

Labels
None yet
Projects
None yet
Milestone
3.1.1
Development

Successfully merging this pull request may close these issues.

Improve "did you mean" message when meaning to use object singleton type
6 participants
@bjornregnell @SethTisue @griggt @michelou @prolativ @Kordyjan