[TypeChecker] setInvalid()s cleanup
#31086
Conversation
CodaFi
left a comment
CodaFi
left a comment
There was a problem hiding this comment.
Please only limit the removal to the part of TypeCheckDeclPrimary we discussed earlier. Some of these tests have regressed in diagnostic quality because of this change.
I'm sorry, I thought you mean remove all of the setInvalid()s occurrences in the file. Anyways, I'll get back to it. However, what should I do about https://github.com/apple/swift/blob/master/test/Parse/enum.swift#L367? |
|
If there's a crash because of a structural problem in the type checker, we need to address that at a more fundamental level. That may involve fixing up TypeCheckPattern to be more tolerant of invalid code. |
This reverts commit 2effbb7.
|
Because of all of the regression that happened, I decided to remove only the The next thing was this test case which was causing a compiler crash
As @CodaFi pointed me, the problem was in the Why the compiler is crashing? I think that's because at My solution Diagnostics regressions Sorry for the long summery, but I just wanted to share what I came to. :) cc @CodaFi , @LucianoPAlmeida , @xedin |
|
|
||
| if (auto *oe = dyn_cast<EnumElementDecl>(e)) { | ||
| // Ambiguities should be ruled out by parsing. | ||
| assert(!foundElement && "ambiguity in enum case name lookup?!"); |
There was a problem hiding this comment.
We shouldn't just remove this assertion. This amounts to a non-trivial behavior change in this function - it's now going to break ambiguities by selecting the last returned result from a lookup. The trouble here isn't the assert, it's the
if (e->isInvalid()) {
continue;
}
check guarding the filtering behavior. That condition was correct in 2016 when this was written, but is not great now that we've stabilized the computation of interface types and defined isInvalid in terms of it.
There was a problem hiding this comment.
I think this means this code just needs to become tolerant of duplicates (e.g. by diagnosing them?). Quite what that means exactly I'm not sure - I need to go page all of this back in.
There was a problem hiding this comment.
Does that mean I might need to check for redeclarations in TypeCheckPattern?
There was a problem hiding this comment.
Thank you so much for helping me. And Ramadan Kareem! 🙏 :)
CodaFi
left a comment
CodaFi
left a comment
There was a problem hiding this comment.
We're getting closer. I think you may have fallen down a bit of a rabbit hole - but that's not unexpected given how old this code is!
|
Regardless, let's see what happens, eh? @swift-ci test |
|
@swift-ci test source compatibility |
|
Build failed |
|
Huh, @swift-ci test source compatibility Debug |
5 participants
In this PR I removed all of the
setInvalid()s from the TypeCheckDeclPrimary file and modified all of the test messages as discussed in both of these comments:<unknown>diagnostic location regardingCodablederived conformances #30824 (comment)<unknown>diagnostic location regardingCodablederived conformances #30824 (comment)All of the tests pass locally on Ubuntu, however, there's only one test that I wasn't able to make it work and it's this test in Parse/enum.swift
I found out that the compiler crashes because of removing only the
setInvalid()in the TypeCheckDeclPrimary at L721 If I didn't remove it this test will pass, however, the diagnostic will change for all of the other tests of course. I was thinking of removing this test which is causing the headache but of course, I'm sure that this is not the correct way to do it :)How can I solve this?
cc @CodaFi @xedin @LucianoPAlmeida
#30824 aka #31037 Follow-up.