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

compiler: resolve panic in substituteTypeArgs Function for *ast.IndexListExpr #142

Closed
wants to merge 1 commit into from

Conversation

iamrajiv
Copy link

Fixes: #141

This PR addresses a panic occurring in the substituteTypeArgs function within the types.go file. The panic is caused by the lack of handling for the *ast.IndexListExpr type, resulting in unexpected behavior during type substitution.

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @iamrajiv !

Do you think you could add a test in https://github.com/stealthrocket/coroutine/blob/main/compiler/testdata/coroutine.go with a code snippet that was triggering the problem?

@iamrajiv
Copy link
Author

I don't think @achille-roussel that we need a test for this because there are other cases in substituteTypeArgs for which tests were not added, so I'll just follow the same approach of not adding tests. But if you want, I can write a test for all the cases defined in the substituteTypeArgs function.

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

A test serves two purposes, it confirms that the change fixes the original problem, and it helps prevent regressions that would reintroduce the issues in the future.

Our test suites defines the set of program constructs that we support. Since the test suite passed before this change, it indicates that we were not testing with a program that contained a *ast.IndexListExpr, to guarantee that we support compiling programs with those constructs, we need to exercise this code path in a test.

@achille-roussel
Copy link
Contributor

Also to follow up on this:

there are other cases in substituteTypeArgs for which tests were not added

The git history may look like that was the case, but it's missing context about in-person development that happened at the time the project was being bootstrapped before it was made public. In general, we have strong test coverage for the features we support, and we maintain a rigorous testing culture to ensure we can continue to provide a high quality solution over time 👍

@chriso
Copy link
Contributor

chriso commented Jun 17, 2024

Fixed by #148

@chriso chriso closed this Jun 17, 2024
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.

compiler: not implemented handling of *ast.IndexListExpr in substituteTypeArgs function
3 participants