Skip to content

Fix miscounting the number of parameters when an issue is recorded. #4

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2023
Merged

Fix miscounting the number of parameters when an issue is recorded. #4

merged 1 commit into from
Sep 21, 2023

Conversation

grynspan
Copy link
Contributor

We're accidentally reporting the number of characters in the string representation of each argument as the number of arguments.

For example, with a 1-arg function, we're currently emitting something like:

✘ Test f(i:) recorded an issue with 6 arguments i → 10 at fooTests.swift:13:3: Expectation failed: (i → 10) < 10

When we should be emitting:

✘ Test f(i:) recorded an issue with 1 argument i → 10 at fooTests.swift:13:3: Expectation failed: (i → 10) < 10

@grynspan
Fix miscounting the number of parameters when an issue is recorded.
6f65917 
We're accidentally reporting the number of characters in the string representation of each argument as the number of arguments.
@grynspan
Copy link
Contributor Author

@swift-ci please test

@grynspan grynspan merged commit 15df2de into main Sep 21, 2023
@grynspan grynspan deleted the jgrynspan/fix-parameter-miscount-on-recorded-issue branch September 21, 2023 18:03
grynspan added a commit that referenced this pull request Oct 2, 2024
@grynspan
Disable reformatting of macro expansions.
1b1179e 
This PR turns off automatic reformatting of our macro expansions. I noticed that
at-desk, the compiler was spending an inordinate amount of time in reformatting:

```
Call graph:
    7149 Thread_709950   DispatchQueue_1: com.apple.main-thread  (serial)
      7149 start  (in dyld) + 2840  [0x18dafc274]
        7149 main  (in TestingMacros) + 28  [0x1029291d4]  TestingMacrosMain.swift:18
          7149 static TestingMacrosMain.$main()  (in TestingMacros) + 160  [0x1029290c4]  /<compiler-generated>:0
            7149 static CompilerPlugin.main()  (in TestingMacros) + 544  [0x10337f64c]  CompilerPlugin.swift:115
              6702 CompilerPluginMessageListener.main()  (in TestingMacros) + 536  [0x1033cc85c]  CompilerPluginMessageHandler.swift:96
              + 6692 CompilerPluginMessageHandler.handleMessage(_:)  (in TestingMacros) + 2320  [0x1033ce32c]  CompilerPluginMessageHandler.swift:169
              + ! 6375 CompilerPluginMessageHandler.expandAttachedMacro(macro:macroRole:discriminator:attributeSyntax:declSyntax:parentDeclSyntax:extendedTypeSyntax:conformanceListSyntax:lexicalContext:)  (in TestingMacros) + 1572  [0x1033fec00]  Macros.swift:155
              + ! : 3284 expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 3132  [0x1033a96d0]  MacroExpansion.swift:280
              + ! : | 3284 Collection.map<A, B>(_:)  (in TestingMacros) + 1656  [0x1033a7eec]  /<compiler-generated>:0
              + ! : |   3284 partial apply for closure #4 in expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 36  [0x1033ac83c]  /<compiler-generated>:0
              + ! : |     3284 closure #4 in expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 180  [0x1033aaaa8]  MacroExpansion.swift:281
              + ! : |       3243 SyntaxProtocol.formattedExpansion(_:indentationWidth:)  (in TestingMacros) + 188  [0x1033a81f4]  MacroExpansion.swift:409
`

3243 samples is significantly more than what we spend in test macro expansion:

```
+ ! : 2277 expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 2932  [0x1033a9608]  MacroExpansion.swift:273
+ ! : | 2277 protocol witness for static PeerMacro.expansion<A, B>(of:providingPeersOf:in:) in conformance TestDeclarationMacro  (in TestingMacros) + 20  [0x102928ad0]  /<compiler-generated>:0
+ ! : |   2258 static TestDeclarationMacro.expansion<A, B>(of:providingPeersOf:in:)  (in TestingMacros) + 316  [0x10291f73c]  TestDeclarationMacro.swift:31
+ ! : |   + 966 static TestDeclarationMacro._createTestContainerDecls<A>(for:on:testAttribute:in:)  (in TestingMacros) + 2184  [0x1029213b8]  TestDeclarationMacro.swift:410
```

These samples are in a debug build—I fully expect the performance to be better
in a release build—but they still significantly impact our build time when used
as a package or tested at desk.

Works around rdar://137132570.
grynspan added a commit that referenced this pull request Oct 2, 2024
@grynspan
Disable reformatting of macro expansions. (#743)
5763213 
This PR turns off automatic reformatting of our macro expansions. I
noticed that at-desk, the compiler was spending an inordinate amount of
time in reformatting:

```
Call graph:
    7149 Thread_709950   DispatchQueue_1: com.apple.main-thread  (serial)
      7149 start  (in dyld) + 2840  [0x18dafc274]
        7149 main  (in TestingMacros) + 28  [0x1029291d4]  TestingMacrosMain.swift:18
          7149 static TestingMacrosMain.$main()  (in TestingMacros) + 160  [0x1029290c4]  /<compiler-generated>:0
            7149 static CompilerPlugin.main()  (in TestingMacros) + 544  [0x10337f64c]  CompilerPlugin.swift:115
              6702 CompilerPluginMessageListener.main()  (in TestingMacros) + 536  [0x1033cc85c]  CompilerPluginMessageHandler.swift:96
              + 6692 CompilerPluginMessageHandler.handleMessage(_:)  (in TestingMacros) + 2320  [0x1033ce32c]  CompilerPluginMessageHandler.swift:169
              + ! 6375 CompilerPluginMessageHandler.expandAttachedMacro(macro:macroRole:discriminator:attributeSyntax:declSyntax:parentDeclSyntax:extendedTypeSyntax:conformanceListSyntax:lexicalContext:)  (in TestingMacros) + 1572  [0x1033fec00]  Macros.swift:155
              + ! : 3284 expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 3132  [0x1033a96d0]  MacroExpansion.swift:280
              + ! : | 3284 Collection.map<A, B>(_:)  (in TestingMacros) + 1656  [0x1033a7eec]  /<compiler-generated>:0
              + ! : |   3284 partial apply for closure #4 in expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 36  [0x1033ac83c]  /<compiler-generated>:0
              + ! : |     3284 closure #4 in expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 180  [0x1033aaaa8]  MacroExpansion.swift:281
              + ! : |       3243 SyntaxProtocol.formattedExpansion(_:indentationWidth:)  (in TestingMacros) + 188  [0x1033a81f4]  MacroExpansion.swift:409
```

3243 samples is significantly more than what we spend in test macro
expansion:

```
+ ! : 2277 expandAttachedMacroWithoutCollapsing<A>(definition:macroRole:attributeNode:declarationNode:parentDeclNode:extendedType:conformanceList:in:indentationWidth:)  (in TestingMacros) + 2932  [0x1033a9608]  MacroExpansion.swift:273
+ ! : | 2277 protocol witness for static PeerMacro.expansion<A, B>(of:providingPeersOf:in:) in conformance TestDeclarationMacro  (in TestingMacros) + 20  [0x102928ad0]  /<compiler-generated>:0
+ ! : |   2258 static TestDeclarationMacro.expansion<A, B>(of:providingPeersOf:in:)  (in TestingMacros) + 316  [0x10291f73c]  TestDeclarationMacro.swift:31
+ ! : |   + 966 static TestDeclarationMacro._createTestContainerDecls<A>(for:on:testAttribute:in:)  (in TestingMacros) + 2184  [0x1029213b8]  TestDeclarationMacro.swift:410
```

These samples are in a debug build—I fully expect the performance to be
better
in a release build—but they still significantly impact our build time
when used
as a package or tested at desk.

Works around rdar://137132570.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennisweissmann dennisweissmann dennisweissmann approved these changes

@stmontgomery stmontgomery Awaiting requested review from stmontgomery stmontgomery is a code owner

@briancroom briancroom Awaiting requested review from briancroom briancroom is a code owner

@SeanROlszewski SeanROlszewski Awaiting requested review from SeanROlszewski

@suzannaratcliff suzannaratcliff Awaiting requested review from suzannaratcliff suzannaratcliff is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grynspan @dennisweissmann