[SyntaxBuilder] Add default conformance to syntax collection elements #40120
Conversation
utils/gyb_syntax_support/__init__.py
Outdated
| @@ -179,3 +180,27 @@ def _digest_trivia(trivia): | |||
| _digest_trivia(trivia) | |||
|
|
|||
| return digest.hexdigest() | |||
|
|
|||
|
|
|||
| def create_syntax_buildable_expressible_as_syntax_collection_conformances(): | |||
There was a problem hiding this comment.
IIUC this generates a map that looks as follows:
{
'ConditionElement': [
'ExpressibleAsConditionElementList'
],
…
}Please correct me if I’m wrong.
I’ve got a couple suggestions for this
- Could you add a doc comment that shows the structure of the generated map by an example
- Wouldn’t it be cleaner if it didn’t use
ExpressibleAs*types as elements? - If the function just generates a mapping of syntax nodes to the collections they may appear as a child in, I think a more descriptive name for the function would be something like
syntax_collection_element_to_collection_mapping.
There was a problem hiding this comment.
Could you add a doc comment that shows the structure of the generated map by an example
Added.
Wouldn’t it be cleaner if it didn’t use
ExpressibleAs*types as elements?
The idea was to have the same list as in https://github.com/apple/swift/blob/06039917bda350f8832ce56590d5691329f96075/utils/gyb_syntax_support/protocolsMap.py#L3
If don't add ExpressibleAs* here, we need to add in this method, then we also need to remove it in protocolsMap.py to be able to use the same logic as now in https://github.com/apple/swift-syntax/pull/334/files#diff-1692562863ad5b0153cfede0d8b3fa518b3792f644fa2425ad41b1ac2f9ed33fR46
If the function just generates a mapping of syntax nodes to the collections they may appear as a child in, I think a more descriptive name for the function would be something like
syntax_collection_element_to_collection_mapping.
Done
There was a problem hiding this comment.
The idea was to have the same list as in
If don't add
ExpressibleAs*here, we need to add in this method, then we also need to remove it in protocolsMap.py to be able to use the same logic as now in https://github.com/apple/swift-syntax/pull/334/files#diff-1692562863ad5b0153cfede0d8b3fa518b3792f644fa2425ad41b1ac2f9ed33fR46
That makes sense. I don’t have too strong opinions on whether the generated map looks like
{
'ConditionElement': ['ConditionElementList']
}or
{
'ExpressibleAsConditionElement': ['ExpressibleAsConditionElementList']
}I just didn’t like the mixed approach where the keys weren’t ExpressibleAs* types but the values were ExpressibleAs*. And IIUC the old version generated that mixed version.
Having the function generate a map that’s compatible with protocolsMap.py seems very reasonable to me.
There was a problem hiding this comment.
I updated the protocolsMap.py.
Should we change the name as it is SYNTAX_BUILDABLE_EXPRESSIBLE_BY_CONFORMANCES now?
There was a problem hiding this comment.
I think the name is still fine. It does effectively add conformances to ExpressibleBy and not to the raw buildable types.
kimdv
force-pushed
the
kimdv/add-default-syntax-collection-conformance
branch
6 times, most recently
from
6d050a4 to
da6b762
Compare
kimdv
force-pushed
the
kimdv/add-default-syntax-collection-conformance
branch
2 times, most recently
from
90231c2 to
6ba9f20
Compare
kimdv
force-pushed
the
kimdv/add-default-syntax-collection-conformance
branch
from
6ba9f20 to
8b13466
Compare
kimdv
force-pushed
the
kimdv/add-default-syntax-collection-conformance
branch
from
8b13466 to
3b307b7
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
@ahoppen I got write access now. |
|
Congratulations 🎉 Yes, that’s correct. Feel free to also trigger CI when you think the PR is ready. |
swiftlang/swift-syntax#334