[AST] NFC: Formalize Decl validation tracking via RAII #16655
Conversation
|
@swift-ci please smoke test |
|
I like the idea of using RAII to set some of these new flags, but I’m not a fan of the renaming. |
|
What would you propose then for the names? The problem as I see it was twofold:
|
I'm not sure that's why. The "is being validated" flag exists to catch re-entrant calls to validateDecl() from taking place. If the declaration is already being validated or has completed validation, we want to return. Otherwise, we can begin validation. The "validation completed" state is distinct from "has interface type" because type declarations get an interface type before getting a generic signature. |
davezarzycki
force-pushed
the
formalize_DeclIsBeingValidatedRAII
branch
from
7a4a062 to
f5ae64a
Compare
|
Hi @slavapestov – I think your feedback was fair. I "shot from the hip" with this pull request. I've now done a more thorough review. @swift-ci please smoke test |
|
The failure looks unrelated. @swift-ci please clean smoke test |
|
@swift-ci please smoke test |
|
Ping. |
|
Ping. Are there any changes that are wanted? |
|
Ping. |
|
Ping. It has been almost a month. Should I abandon this pull request? |
After this change, RAII ensures that the validation state is accurate as possible.
davezarzycki
force-pushed
the
formalize_DeclIsBeingValidatedRAII
branch
from
f5ae64a to
b29d278
Compare
|
Hi @DougGregor – If you could be so kind, how should I interpret the lack of responses to "pings"? @swift-ci please test |
|
@slavapestov and @DougGregor – Ping. Feedback would be appreciated. Thanks! |
|
@davezarzycki What is the concrete benefit of this change? Are you using the new validation state for anything? In the long term, we would like to switch everything over to use the request evaluator, and validateDecl(), together with these state bits, will go away entirely. They're too imprecise and error prone. |
include/swift/AST/Decl.h
Outdated
| case ValidationState::Unchecked: | ||
| case ValidationState::Checked: | ||
| return false; | ||
| default: |
There was a problem hiding this comment.
How about listing out the two cases here, rather than using default?
| // See how validateDecl() sets the generic environment on alias members | ||
| // explicitly. | ||
| // | ||
| // FIXME: Hopefully this can all go away with the ITC. |
There was a problem hiding this comment.
The ITC is now gone. Our hopes and dreams are pinned on the request-evaluator ;)
|
I agree that the current validation tracking is imprecise and error prone. That is why I thought that RAII seemed like a natural way of trending in the right direction. If the validation tracking can go away, that's even better. What do you want to do in the short to medium term? For the record, I'm not using the improved tracking in this pull request for feature work. Just better/easier compiler debugging. |
|
The use of RAII here is adding more discipline to a messy area. While it's true that we expect this mess of state to go away with the request-evaluator (once we've peeled off enough requests that |
| @@ -3666,6 +3664,8 @@ void TypeChecker::validateDecl(ValueDecl *D) { | |||
| // up to the caller, who must call hasValidSignature() to | |||
| // check that validateDecl() returned a fully-formed decl. | |||
| if (D->hasValidationStarted()) { | |||
| if (!(D->isBeingValidated() || D->hasValidSignature())) | |||
| D->dump(); | |||
There was a problem hiding this comment.
I just noticed this call to dump() after fetching the latest sources and seeing a build warning.
There was a problem hiding this comment.
Sorry about that. @slavapestov has a pull request that removes it: #18025
After this change, RAII ensures that the validation state is accurate as possible.