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 futures #28469

Open
wants to merge 3 commits into
base: mainnet
Choose a base branch
from
Open

Fix futures #28469

wants to merge 3 commits into from

Conversation

mikebenfield
Copy link
Collaborator

No description provided.

Also, fix the span for function inputs.
@mikebenfield mikebenfield requested a review from d0cd December 3, 2024 19:21
This way we don't get misleading error messages.
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

Looks good overall, left some nits and comments.
Any chance this approach can be used to address either of these issues:

I believe the CI failure is due to some expectations that need to be regenerated

Cargo.lock Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we update the lockfile? One implication of doing this often is that the CI caches are invalidated every time the lockfile changes.

compiler/ast/src/passes/visitor.rs Outdated Show resolved Hide resolved
errors/src/errors/static_analyzer/static_analyzer_error.rs Outdated Show resolved Hide resolved
}

let ty: Type = if is_future {
inferred_type.expect("Type checking guarantees the inferred type is present")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit. Could we use the unreachable macro here? This helps us look back on those sorts of invariants.

@mikebenfield
Copy link
Collaborator Author

Indeed, this issue is now fixed (and I've updated the last commit message to refer to it).

And I think this one is too. Maybe. The compiler will now give an error for the code in the example, because you can't assign a Future out of a tuple to a new variable like that. I think that suffices to fix this issue, but maybe there is still a problem around type checking futures?

Now futures are tracked properly, whether they're stored directly
in a variable or in a tuple.

Also, error if a future is used improperly.

Fixes #28471
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.

2 participants