Skip to content

Switch RecursionId to any #1477

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jakebailey
Copy link
Member

This code is subtly wrong because some of the IDs are uint64 and are being converted to uint32. In long running programs, this would manifest as very strange behavior.

I don't think we actually need anything special here; any is enough to do the right thing and avoids making that same mistake in the same amount of space as it would take to expand the struct to have a uint64 value + padding.

@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 04:15
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the RecursionId type by replacing a structured approach with a simple any type to fix potential data truncation issues. The change addresses a bug where uint64 IDs were being incorrectly cast to uint32, which could cause issues in long-running programs.

Key changes:

  • Remove the RecursionIdKind enum and RecursionId struct
  • Replace with a simple RecursionId any type alias
  • Update all usage sites to store objects directly instead of extracting and casting their IDs
Comments suppressed due to low confidence (1)

internal/checker/relater.go:89

  • [nitpick] The type alias RecursionId any could be confusing since it doesn't provide any type safety or documentation about what values are expected. Consider using a more descriptive name or adding a comment explaining what types of values this should contain.
type RecursionId any

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.

1 participant