Skip to content

Report the correct errors in the IDE and add test infrastructure for diagnostics #5484

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
Merged
merged 6 commits into from
Nov 21, 2018
Merged

Report the correct errors in the IDE and add test infrastructure for diagnostics #5484

merged 6 commits into from
Nov 21, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Nov 20, 2018

No description provided.

@smarter smarter added this to the 0.11 Tech Preview milestone Nov 20, 2018
@smarter smarter requested review from odersky and Duhemm November 20, 2018 19:50
@smarter
Copy link
Member Author

smarter commented Nov 20, 2018

@Duhemm Could you have a look at the HoverTest failure in f239b67 ? I don't understand what's going on.

@Duhemm
Copy link
Contributor

Duhemm commented Nov 21, 2018

Interesting. Because the code didn't pass frontend, it would never reach cookComments. The expandedBody of comments are stripped of their @usecase and @define sections. We can use expanded instead, which doesn't discard them. I've pushed a fix.

Duhemm
Duhemm reviewed Nov 21, 2018
View reviewed changes
Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

Nice! Could you also add a test for the pure expression in warning position in the worksheets?

language-server/test/dotty/tools/languageserver/util/CodeTester.scala
assertEquals(expectedParams, actualParams)

this
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The other "actions" in this file usually do

def foo(marker: CodeMarker, expected: ...) = doAction(new Foo(marker, expected))

Any reason for not following the same pattern?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly because there's no action (request sent to the server) involved here, the request is "didOpen" but it's done implicitly. But if you think this could be refactored, please do!

@smarter
Copy link
Member Author

smarter commented Nov 21, 2018

The expandedBody of comments are stripped of their @usecase and @define sections. We can use expanded instead, which doesn't discard them

I see, but maybe the documentation we show to the user should not contain these sections anyway, since they're basically implementation details ?

@Duhemm
Copy link
Contributor

Duhemm commented Nov 21, 2018

As it is now, the @define will not be shown (no knownTag is associated to it). Just revert my commit and remove @usecase from knownTags in ParsedComment to hide @usecase.

odersky
odersky approved these changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@odersky odersky removed their assignment Nov 21, 2018
@smarter
Don't display usecases in IDE/REPL
7469d59 
It's not really useful and can be confusing.
@smarter
IDE test: add missing stripMargin for code interpolator
a3e8e95 
This means that many tests were failing to compile previously, but we
never noticed.

FIXME: Somehow, this broke the documentation test in
HoverTest (the @usecase part of the doc is missing from the output now)
@smarter
IDE: add test infrastructure for diagnostics
15d9af3
@smarter
Report the correct errors in the IDE
38c6d46 
In some situations (like when interpolating type variables) the logic
changes depending on whether we have unreported errors or not.
Before this commit, we assumed that any error in a StoreReporter was
unreported, but this is wrong: the IDE, the REPL and other tools use
a StoreReporter as their root reporter.

This commit fixes this by making sure all the code that looks for
unreported errors does so by calling `hasUnreportedErrors` and
making that method return false when there is no outer reporter.
@smarter
Add diagnostics test for "pure expression" warning
d180287 
Also do not reuse the compiler options used in the Dotty build for the
tests, since they can influence their result (e.g. -Xfatal-warnings)
@smarter
Copy link
Member Author

smarter commented Nov 21, 2018

@Duhemm Good to merge ?

@Duhemm Duhemm merged commit a967a25 into scala:master Nov 21, 2018
@Duhemm Duhemm deleted the fix-error-pos-2 branch November 21, 2018 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

@Duhemm Duhemm Duhemm approved these changes

Assignees

@Duhemm Duhemm

Labels
None yet
Projects
None yet
Milestone
0.11 Tech Preview
Development

Successfully merging this pull request may close these issues.
3 participants
@smarter @Duhemm @odersky