-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Sema] Introduce a couple of ExprPattern requests #64174
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
Conversation
This replaces `synthesizeTildeEqualsOperatorApplication`, and synthesizes the match expression and var on-demand. Additionally, it pushes the lookup logic into pre-checking.
It's not clear to me why it was ever set this way, but it prevents IUOs from working with the match operator. Unset it, and let pre-checking correctly assign it a `FunctionRefKind::SingleApply`.
This is needed to ensure we only ever synthesize a single unique ExprPattern when solving an EnumElementPattern that we failed to lookup a member for.
@swift-ci please test |
@swift-ci please test source compatibility |
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.
Looks great, thank you!
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.
LGTM
@@ -214,7 +214,8 @@ deriveBodyCodingKey_enum_stringValue(AbstractFunctionDecl *strValDecl, void *) { | |||
for (auto *elt : elements) { | |||
auto *baseTE = TypeExpr::createImplicit(enumType, C); | |||
auto *pat = new (C) EnumElementPattern(baseTE, SourceLoc(), DeclNameLoc(), | |||
DeclNameRef(), elt, nullptr); | |||
DeclNameRef(), elt, nullptr, | |||
/*DC*/ strValDecl); |
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.
Nit: I think the preferred style for these kinds of named arguments is the following, which is detected by clang-format
and removes the space after the comment.
Same below
/*DC*/ strValDecl); | |
/*DC=*/strValDecl); |
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.
Seems like clang-format is happy with either style, if the =
isn't present, it inserts a space, otherwise it removes it. Both styles seem to be widely in use in the codebase:
swift on pattern-requests
❯ rg "/\*\w+\*/" lib | wc -l
2331
swift on pattern-requests
❯ rg "/\*\w+=\*/" lib | wc -l
2893
Personally I much prefer the variant without the =
and with the space, it feels less claustrophobic. https://llvm.org/docs/CodingStandards.html#comment-formatting does give an example of a comment that uses the =
, but it's unclear if it's saying that it's the preferred style.
I can change this if you want, but I'll do it as a follow-up.
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 prefer /*<ID>=*/
because that's what we mostly use in the solver but I don't usually pay much attention to formatting there.
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.
Seems like clang-format is happy with either style, if the = isn't present, it inserts a space, otherwise it removes it.
FWIW I'm pretty sure the "happy with either style" is really just because it adds whitespace around /**/
normally (as opposed to the special case when it has =
). I'm surprised how many non-= there are, I've basically always seen = 🤔
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 it varies by component, e.g:
With = |
Without = |
|
---|---|---|
IDE | 170 | 41 |
IRGen | 420 | 144 |
SILOptimizer | 122 | 260 |
Serialization | 46 | 96 |
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, OK. I didn’t know it was this mixed. It’s not a blocker for me either, just something I noticed. I’m happy to merge it as-is as well.
Introduce a request to synthesize the
~=
application for an ExprPattern, and a request to synthesize an ExprPattern for the fallback type-checking of an EnumElementPattern when we are unable to lookup its member. This has been split off #63963.