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

Fix double-nesting of Note constructors #1849

Merged
merged 2 commits into from
Jun 13, 2020
Merged

Fix double-nesting of Note constructors #1849

merged 2 commits into from
Jun 13, 2020

Conversation

Gabriella439
Copy link
Collaborator

Fixes #1784

In #1824 I introduced a bug in computing source spans for Note constructors.
The (very indirect) consequence of this bug was that doubly-nested Note
constructors would show up in the syntax tree.

For example, the following command would generate far more Note
constructors than you would expect before this fix:

$ dhall haskell-syntax-tree --noted <<< 'A B'

Among other things, that broke the LSP server, which assumes that
Note constructors are only nested once.

I added a regression test to prevent this from recurring in the
future.

Fixes #1784

In #1824 I introduced a bug in computing source spans for `Note` constructors.
The (very indirect) consequence of this bug was that doubly-nested `Note`
constructors would show up in the syntax tree.

For example, the following command would generate *far* more `Note`
constructors than you would expect before this fix:

```bash
$ dhall haskell-syntax-tree --noted <<< 'A B'
```

Among other things, that broke the LSP server, which assumes that
`Note` constructors are only nested once.

I added a regression test to prevent this from recurring in the
future.
@TristanCacqueray
Copy link
Collaborator

The test failure was caused by Error: Remote host not found URL: https://test.dhall-lang.org/random-string. Otherwise the change works as described, A B now results in 4 notes (from 16)

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

This part of the code always makes my head spin! 😄

The new test is great though! 👍

@Gabriella439
Copy link
Collaborator Author

I'm going to disable that test for now, since it keeps causing us issues

@mergify mergify bot merged commit 444e5db into master Jun 13, 2020
@mergify mergify bot deleted the gabriel/fix_notes_2 branch June 13, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSP Freeze Import broken with imports with fallback
3 participants