[AST] Remove ParenType #59141
Merged
[AST] Remove ParenType #59141
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hamishknight
force-pushed
the
wrapping-paper
branch
from
58a21d3 to
570ab52
Compare
hamishknight
force-pushed
the
wrapping-paper
branch
from
570ab52 to
f3bbcc2
Compare
hamishknight
force-pushed
the
wrapping-paper
branch
2 times, most recently
from
c340b65 to
2ca6d57
Compare
hamishknight
force-pushed
the
wrapping-paper
branch
from
2ca6d57 to
1218054
Compare
hamishknight
requested review from
hborla,
slavapestov,
xedin,
AnthonyLatsis,
jckarter,
ahoppen,
bnbarham,
rintaro,
adrian-prantl,
ktoso,
xymus,
zoecarver,
hyp,
egorzhdan,
Xazax-hun,
beccadax,
ian-twilightcoder,
tshortli and
rjmccall
as code owners
hamishknight
commented
Oct 7, 2024
hamishknight
commented
Oct 7, 2024
Comment on lines
+5891
to
+5950
| // Note we don't look through parens. | ||
| ExistentialMetatypeType *existentialTy = nullptr; | ||
| if (!constraintRepr->isParenType()) | ||
| existentialTy = dyn_cast<ExistentialMetatypeType>(ty.get().getPointer()); |
There was a problem hiding this comment.
@kavon Is it intentional that this doesn't look through parens, e.g we accept any ~Copyable.Type, but not any ~(Copyable.Type)? For now I've just maintained the current behavior
hamishknight
force-pushed
the
wrapping-paper
branch
from
1218054 to
c231330
Compare
This was referenced Oct 24, 2024
|
Split out a couple of PRs to hopefully make this a bit easier to land: |
hamishknight
force-pushed
the
wrapping-paper
branch
2 times, most recently
from
cfc3f53 to
8a5654b
Compare
slavapestov
approved these changes
Oct 29, 2024
| @@ -1470,16 +1470,6 @@ class TypeDecoder { | |||
|
|
|||
| return Builder.createDictionaryType(key.getType(), value.getType()); | |||
| } | |||
| case NodeKind::SugaredParen: { | |||
There was a problem hiding this comment.
We might want to keep some minimal demangler support just in case it's needed for backwards compatibility with existing debug info; just have this return base.getType() on line 1481
There was a problem hiding this comment.
Ah okay, I guess that means we also need to keep the logic in Demangler.cpp?
There was a problem hiding this comment.
Updated with the restored demangling code
hamishknight
force-pushed
the
wrapping-paper
branch
from
8a5654b to
7ef6c0c
Compare
|
@swift-ci please test source compatibility |
hamishknight
force-pushed
the
wrapping-paper
branch
from
7ef6c0c to
f890d22
Compare
…arens Check for `isParenType` on the TypeRepr to avoid relying on getting that behavior by casting the type pointer.
I missed this in my other Sema ParenType PR, avoid relying on ParenType here.
Today ParenType is used: 1. As the type of ParenExpr 2. As the payload type of an unlabeled single associated value enum case (and the type of ParenPattern). 3. As the type for an `(X)` TypeRepr For 1, this leads to some odd behavior, e.g the type of `(5.0 * 5).squareRoot()` is `(Double)`. For 2, we should be checking the arity of the enum case constructor parameters and the presence of ParenPattern respectively. Eventually we ought to consider replacing Paren/TuplePattern with a PatternList node, similar to ArgumentList. 3 is one case where it could be argued that there's some utility in preserving the sugar of the type that the user wrote. However it's really not clear to me that this is particularly desirable since a bunch of diagnostic logic is already stripping ParenTypes. In cases where we care about how the type was written in source, we really ought to be consulting the TypeRepr.
hamishknight
force-pushed
the
wrapping-paper
branch
from
f890d22 to
2d7500e
Compare
|
@swift-ci please test |
xedin
approved these changes
Oct 31, 2024
|
@swift-ci please test macOS |
|
🚢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Today ParenType is used:
(X)TypeReprFor 1, this leads to some odd behavior, e.g the type of
(5.0 * 5).squareRoot()is(Double). For 2, we should be checking the arity of the enum case constructor parameters and the presence of ParenPattern respectively. Eventually we ought to consider replacing Paren/TuplePattern with a PatternList node, similar to ArgumentList.3 is one case where it could be argued that there's some utility in preserving the sugar of the type that the user wrote. However it's really not clear to me that this is particularly desirable since a bunch of diagnostic logic is already stripping ParenTypes. In cases where we care about how the type was written in source, we really ought to be consulting the TypeRepr.