Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix hanging redundant import on Unicode function #2870

Merged
merged 7 commits into from
May 1, 2022

Conversation

drsooch
Copy link
Collaborator

@drsooch drsooch commented Apr 30, 2022

Closes #2859

Revert partial changes from #2483

For redundant import actions, the code path ends up calling wrapOperatorInParen which under the hood makes a call to is_ident which is a low-level Char -> Bool. The function will panic when it's input falls out of ASCII range.

The fix is just to match directly on the '_' character. Outside of this codepath, the original
functionality has been restored (using isAlpha).

Also removes the now unnecessary is_ident compat layer.

@drsooch drsooch requested a review from pepeiborra as a code owner April 30, 2022 04:12
Instead of using a low-level `is_ident` that only works on ascii
characters. Instead match directly on the '_' character. The original
functionality has been restored (using `isAlpha` instead).

This commit removes the now unneccessary `is_ident` compat layer
@drsooch drsooch force-pushed the fix-unicode-redundant-imports branch from 6179208 to 528a8bd Compare April 30, 2022 04:13
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Is it worth adding a test?

@konn
Copy link
Collaborator

konn commented Apr 30, 2022

Is it worth adding a test?

I think that it makes sense to add a simple test based on the repro I reported in #2859 to make sure that the reported issue is actually solved by this PR.

@drsooch
Copy link
Collaborator Author

drsooch commented Apr 30, 2022

Will add one yes

@drsooch
Copy link
Collaborator Author

drsooch commented Apr 30, 2022

Regression test properly fails. Will timeout after some time, hope this is reasonable?

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Apr 30, 2022
@mergify mergify bot merged commit e6aa0aa into haskell:master May 1, 2022
@drsooch drsooch deleted the fix-unicode-redundant-imports branch May 1, 2022 12:44
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request May 21, 2022
* Revert partial changes from haskell#2483

Instead of using a low-level `is_ident` that only works on ascii
characters. Instead match directly on the '_' character. The original
functionality has been restored (using `isAlpha` instead).

This commit removes the now unneccessary `is_ident` compat layer

* Add regression test for unicode functions

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
hololeap pushed a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
* Revert partial changes from haskell#2483

Instead of using a low-level `is_ident` that only works on ascii
characters. Instead match directly on the '_' character. The original
functionality has been restored (using `isAlpha` instead).

This commit removes the now unneccessary `is_ident` compat layer

* Add regression test for unicode functions

Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant symbol containing Unicode character may cause HLS to hang
4 participants