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

Do not query interface's self_param_id unless defined when importing #4137

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

gysddn
Copy link
Contributor

@gysddn gysddn commented Jul 15, 2024

Querying a local constant with an invalid instruction triggers a crash, this will prevent self_param_id to be used unless it is defined.

Closes #4071 and #4080

This will only fix the crash. The scenario where a declaration-only interface is imported then defined still gives an error message, but it won't crash this time.

Querying a local constant with an invalid instruction triggers a crash,
this will prevent self_param_id to be used unless it is defined.

Fixes carbon-language#4071 and carbon-language#4080
@github-actions github-actions bot requested a review from zygoloid July 15, 2024 23:18
zygoloid
zygoloid previously approved these changes Jul 15, 2024
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Changes look good, but please can you also add a test?

@zygoloid zygoloid self-requested a review July 15, 2024 23:53
@zygoloid zygoloid dismissed their stale review July 15, 2024 23:54

Would like a test to be added before merging.

- Covers the test cases in carbon-language#4071 and carbon-language#4080
- Importing declaration works without an issue
- Importing, then defining the interface throws a duplicate declaration
issue since it is not yet implemented.
@gysddn
Copy link
Contributor Author

gysddn commented Jul 16, 2024

Added two cases to cover both issues 👍🏻

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Thank you!

@zygoloid zygoloid enabled auto-merge July 16, 2024 16:47
@zygoloid zygoloid added this pull request to the merge queue Jul 16, 2024
Merged via the queue into carbon-language:trunk with commit 5edd235 Jul 16, 2024
7 checks passed
brymer-meneses pushed a commit to brymer-meneses/carbon-lang that referenced this pull request Aug 15, 2024
…arbon-language#4137)

Querying a local constant with an invalid instruction triggers a crash,
this will prevent self_param_id to be used unless it is defined.

Closes carbon-language#4071 and carbon-language#4080

This will only fix the crash. The scenario where a declaration-only
interface is imported then defined still gives an error message, but it
won't crash this time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolchain crashes on interface redeclaration
2 participants