Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
ConstKind::Error
and convertErrorHandled::Reported
to it. #71049New 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
Add
ConstKind::Error
and convertErrorHandled::Reported
to it. #71049Changes from all commits
2deb39d
e22d479
d7c4081
77f38dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this case doesn't early return so it results in
Ok(_)
and thenreport_as_lint
will turn it intoErrorReported::Linted
, which seems wrong?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @oli-obk @RalfJung
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function unfortunately became a mess over time. :/ I get the feeling we are trying to shove too many different things into one code path here.
I also do not have the slightest clue what that
replace_span_with
business inreport_as_lint
is even doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
SizeOverflow
will be a hard error even whenreport_as_lint
is called, so always returningLinted
there seems wrong, superficially.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, so the
must_error
variable should be eliminated in favour of early returning here by callingreport_as_error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept
must_error
but replacedResult<(), ErrorHandled>
in the return type withErrorHandled
, let me know if that's enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that works, too, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding that boolean could have made the code cleaner... but maybe not, and anyway we can leave that to later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this temporary and won't always remain so, so let's not bake that assumption too deeply into the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already, this is nothing compared to everything else. It's only "temporary" in the sense that the first thing we stabilize won't have generic parameters in
const
parameter types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the problem is we'd need to stabilize something like
StructuralMatch
(some deep trait, unlikeStructuralPartialEq
/StructuralEq
), because we can't just allow unrestricted types.But even with that also it's a PITA to implement correctly, far more so than just concrete types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I understand all of this; I'm just saying we should avoid too deep of an architectural dependence on the fact that const params can't depend on type params so that it doesn't take a year to dig out of.