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

Damlc ITs fix pretty range #7707

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Damlc ITs fix pretty range #7707

merged 2 commits into from
Oct 16, 2020

Conversation

hurryabit
Copy link
Contributor

@hurryabit hurryabit commented Oct 15, 2020 •
edited
Loading

The new version of ghcide fixes a bug in the pretty printer for
diagnostics. So far, the column number for an error/warning you would
be shown by daml build was always be one smaller than in DAML
Studio. Now, we show the same locations across the command line tools
and the IDE.

We also bring our test files in sync with the ranges the command line
tools print now.

CHANGELOG_BEGIN
[DAML Compiler] Show the correct column numbers in error locations
produced by command line tools like daml build.
CHANGELOG_END

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

This change is Reviewable

@hurryabit
Bump ghcide to newest version
33ddb48 
The new version of `ghcide` fixes a bug in the pretty printer for
diagnostics. So far, the column number for an error/warning you would
be shown by `daml build` was always be one smaller than in DAML
Studio. Now, we show the same locations across the command line tools
and the IDE.

CHANGELOG_BEGIN
[DAML Compiler] Show the correct column numbers in error locations
produced by command line tools like `daml build`.
CHANGELOG_END
hurryabit
hurryabit commented Oct 15, 2020
View reviewed changes
bazel-haskell-deps.bzl
GHCIDE_REV = "30860c8c175732bbc9652b8e6d7dafb354380227"
GHCIDE_SHA256 = "94afc5a3eda790956187080445a76e56b16f2e77c8f5cb3cc3450b6dd019d40a"
GHCIDE_REV = "8e4f52892d88d23259547b03bb096b46f28c0afc"
GHCIDE_SHA256 = "3fa3e23d760f1bdfb2a707d2593ea3eaedba748abfd649e8917c585780fa91db"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is stacked on top of #7706 and needs to be rebased once #7706 has been merged. I'd like to test in CI already now nevertheless.

@hurryabit
Adapt damlc integration tests to newest ghcide
f420bac 
We've recently upgraded to a newer ghcide version in #7706 which fixes
the bug that column numbers in diagnostics printed on the command line
were off by one.

This PR brings the ranges in our test files in line with what the
command line tools print, which is also what vscode shows.

CHANGELOG_BEGIN
CHANGELOG_END
@hurryabit hurryabit force-pushed the damlc-its-fix-pretty-range branch from d45738e to f420bac Compare October 15, 2020 20:58
@hurryabit hurryabit changed the title Damlc its fix pretty range Damlc ITs fix pretty range Oct 15, 2020
cocreature
cocreature approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Looks sensible but the comment about GHC’s location tracking makes me wonder if we’re really making things better by going for 1-based column indexing.

compiler/damlc/tests/src/DA/Test/DamlcIntegration.hs
Comment on lines -342 to -344
-- which are 0-based in both line and column and also open. Error
-- messages are formatted by GHC and are 1-based in line, 0-based
-- in column and closed (so add one to column start, one to
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment about GHC still apply? i.e., will we now get locations in error messages that are 0 based column-wise while the location of the diagnostics containing that error message is 1 based? At the very least it seems worth preserving the comment that documents this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment was never true. GHC's ranges were always 1-based and hence match the way we pretty-print diagnostics now:
https://github.com/digital-asset/daml-ghcide/blob/8e4f52892d88d23259547b03bb096b46f28c0afc/src/Development/IDE/GHC/Error.hs#L70

I think all the confusion came from the bug in the pretty-printer I've fixed yesterday. Since we don't use any SrcSpans in this test but only Ranges, I think the comment makes no sense in this place anymore. In view of #7708, there should also be no more confusion for our devs who want to add or fix tests, since we don't print anything 0-based anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, thanks for the explanation

@hurryabit hurryabit merged commit 44db5de into master Oct 16, 2020
@hurryabit hurryabit deleted the damlc-its-fix-pretty-range branch October 16, 2020 07:06
@hurryabit hurryabit mentioned this pull request Oct 16, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cocreature cocreature cocreature approved these changes

@sofiafaro-da sofiafaro-da Awaiting requested review from sofiafaro-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hurryabit @cocreature