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

salsa 3.0 #1033

Merged
merged 8 commits into from
Jan 17, 2025
Merged

salsa 3.0 #1033

merged 8 commits into from
Jan 17, 2025

Conversation

sbillig
Copy link
Collaborator

@sbillig sbillig commented Jan 13, 2025

@Y-Nak I removed the InputIngot field from InputFile to avoid the infinite loop in the salsa object debug printing. This was a purely mechanical exercise of plumbing the appropriate InputIngot value around where it was needed in place of looking it up from the InputFile. Some of the impacted code could probably use some refactoring.

@micahscopes would you look at the latest commit's changes in the language server code? I added a TODO about a possibly incorrect IngotId being used for subdiagnostics. We probably need to look up the subdiagnostic's span's file's ingot manually there instead.

@Y-Nak Y-Nak mentioned this pull request Jan 15, 2025
Fixes infinite loop in salsa debug printing
@sbillig sbillig marked this pull request as ready for review January 15, 2025 21:14
@sbillig
Copy link
Collaborator Author

sbillig commented Jan 15, 2025

I'd be fine with this being merged as-is, btw. The subdiagnostic ingot thing could be addressed separately.

@sbillig sbillig requested a review from Y-Nak January 15, 2025 21:17
@micahscopes
Copy link
Collaborator

@sbillig I took a look and it seems reasonable. With respect to the subdiagnostic ingot issue, I'm fine with merging as is and adding a followup issue as there is another related cleanup chore that needs to happen: I need to ensure that InputFiles always have the correct path set, a relative path to the input ingot.

Copy link
Member

@Y-Nak Y-Nak left a comment

Choose a reason for hiding this comment

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

👍

@Y-Nak Y-Nak merged commit a9b68f8 into ethereum:fe-v2 Jan 17, 2025
7 checks passed
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