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: Infinite loop when evaluating (()) (#1427) #1456

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Gavinok
Copy link

@Gavinok Gavinok commented Feb 3, 2023

Evaluation of (()) is treated as a malformed expression rather than causing an infinite loop.

Evaluation of `(())` is treated as a malformed expression
rather than causing an infinite loop
@hellerve hellerve requested a review from a team February 3, 2023 07:58
@hellerve
Copy link
Member

hellerve commented Feb 3, 2023

Thanks, this is super useful! 🎉

Can we add a test for this? I suggest we put one in test/test_for_errors where we keep the expected failing code snippets. Then we add the expected output to test/output. See this commit for an example.

@Gavinok
Copy link
Author

Gavinok commented Feb 3, 2023

No problem I am just happy to help. Should be good to go. Let me know if I missed anything

@hellerve
Copy link
Member

hellerve commented Feb 3, 2023

Super! Maybe we should introduce a special error type to improve the error message—or maybe that would be overkill. What does everyone else think?

@Gavinok
Copy link
Author

Gavinok commented Feb 3, 2023

Not exactly sure what a better fitting existing error message would look like. Could try something like the error message of used for applying non functions.

You are trying to call the non-function `()` at line 4, column 2 in 'REPL'. at REPL:4:1.

That's just an idea for reusing an existing error. Don't have any clever unique ideas personally.

@hellerve
Copy link
Member

hellerve commented Feb 3, 2023

Of the two, I personally prefer the non-function error message. But I’d wait for feedback from the others :)

@eriksvedang
Copy link
Collaborator

Very nice! I also prefer the non-function error.

@Gavinok
Copy link
Author

Gavinok commented Feb 18, 2023

@hellerve is there anyone else we are waiting to hear from?

@hellerve
Copy link
Member

No, I think we’re good to merge. @eriksvedang is the only one with those rights, though :)

@eriksvedang
Copy link
Collaborator

The test suite fails on this branch – does it help if you merge in latest master?

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