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

Improve parsing of import suggestions extending multiple multiline imports (fixes #4175) #4177

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Apr 15, 2024

Example ghc error corresponding to test case I'm adding:

Data constructor not in scope: Nothing
Suggested fix:
  Add ‘Nothing’ to one of these import lists:
    ‘Data.Maybe’ (at /home/jhrcek/Tmp/hstest/app/ModuleA.hs:(5,1)-(7,2))
    ‘Prelude’ (at /home/jhrcek/Tmp/hstest/app/ModuleA.hs:(2,1)-(4,2))

#if MIN_VERSION_ghc(9,7,0)
"\\(at ([^)]*)\\)"
Copy link
Collaborator Author

@jhrcek jhrcek Apr 15, 2024

Choose a reason for hiding this comment

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

The original regex didn't correctly deal with import lists spanning multiple lines.
Instead of extracting the whole of

"(/path/to/File.hs:(3,1)-(5,2))"

it would only extract

"(/path/to/File.hs:(3,1)"
                       ^ -only up to first `)`

which led to errors in src span parser described in #4175

@jhrcek jhrcek marked this pull request as ready for review April 15, 2024 06:20
Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

LGTM. We have some new tests to ensure we fix the bug too. I think it is good fix.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, only one suggestion.

More tests are always welcome :)

Comment on lines +1985 to +1987
"\\(at ([^:]+:[^ ]+)\\)"
#else
"\\(([^)]*)\\)"
"\\(([^:]+:[^ ]+)\\)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is almost the same, do you think it is a good idea to remove the CPP and use a regex with an optional group like:
"\\((at )?([^:]+:[^ ]+)\\)"

Copy link
Collaborator Author

@jhrcek jhrcek Apr 15, 2024

Choose a reason for hiding this comment

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

Unfortunately this doesn't work, because it introduces a new capturing group in the regex, which changes the number of matches and because of that it fails the check on this line:

, let result = if length mods == length srcspans then

I thought I could use non-capturing groups (by adding ?: insite the parens), but then I learned the regex library we're using only supports posix-compliant regex syntax, so this is not supported.
I also remember that java had a way of specifying the index of the capturing group whose match you want to retrieve, but this doesn't seem to be supporter by regex-tdfa.. actually it is supported, but I don't think using more complicated regex functionality (getting capturing group by index) outweighs the benefit of removing one CPP 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll wait for further suggestions if you have any and will merge it in the afternoon otherwise.

@jhrcek jhrcek merged commit 64e0acf into master Apr 15, 2024
39 checks passed
@jhrcek jhrcek deleted the jhrcek/fix-extend-multiline-multiimport branch May 11, 2024 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants