-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Delete trust_inference
option
#50432
Conversation
I think the time has come to flip this. This was added when the type system was much less reliable at producing intersections. It is true that we still have the occasional type sytem bug, but we're already trusting inference in a bunch of other places. At the same time, the cost of this has grown in terms of bloated IR needing to be visited in places like irinterp, so let's flip the bit and we'll deal with type system bugs the way we usually due.
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'm fine with this. Can't we just delete this option entirely and remove the related branches also?
I agree with @aviatesk |
Refactored. |
trust_inference
option
Not sure why
|
Known rr bug rr-debugger/rr#3468 |
@@ -630,14 +630,7 @@ function ir_inline_unionsplit!(compact::IncrementalCompact, idx::Int, | |||
end | |||
bb += 1 | |||
# We're now in the fall through block, decide what to do | |||
if fully_covered | |||
if !params.trust_inference | |||
e = Expr(:call, GlobalRef(Core, :throw), FATAL_TYPE_BOUND_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.
Any reason you didn't remove the FATAL_TYPE_BOUND_ERROR
definition?
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.
was this the only use of it? If so the reason is because I didn't notice that there weren't any other uses.
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.
Fix #50709 This issue *appears* fixed on master due to #50432, which simply removed the error. This fixes the underlying cause, which is that we sometimes return typevars in the environment from intersection that have free vars in their bounds. I think it's reasonable to just widen these aggressively, since we usually cannot do much with these kinds of unknown static parameters anyway.
Fix #50709 This issue *appears* fixed on master due to #50432, which simply removed the error. This fixes the underlying cause, which is that we sometimes return typevars in the environment from intersection that have free vars in their bounds. I think it's reasonable to just widen these aggressively, since we usually cannot do much with these kinds of unknown static parameters anyway. (cherry picked from commit a3e2316)
Fix #50709 This issue *appears* fixed on master due to #50432, which simply removed the error. This fixes the underlying cause, which is that we sometimes return typevars in the environment from intersection that have free vars in their bounds. I think it's reasonable to just widen these aggressively, since we usually cannot do much with these kinds of unknown static parameters anyway. (cherry picked from commit a3e2316)
I think the time has come to flip this. This was added when the type system was much less reliable at producing intersections. It is true that we still have the occasional type sytem bug, but we're already trusting inference in a bunch of other places. At the same time, the cost of this has grown in terms of bloated IR needing to be visited in places like irinterp, so let's flip the bit and we'll deal with type system bugs the way we usually due.