[Performance Hints] Add simple check for returning of values of array and dictionary type #84578

artemcm · 2025-09-30T00:18:59Z

Add simple check for returning of values of array and dictionary type
This check will run on each type-checked primary input of the current compilation and emit a warning diagnostic for all discovered occurrences of this code pattern when the performance hints feature is enabled.
Add @performanceOverride attribute to silence individual performance hint diagnostics
This attribute will accept two parameters: a diagnostic group identifier first, and a string literal 'explanation' for the override reason second

aviralg

FunctionReturnType is too generic when we are only checking for arrays and dictionaries. We will implement other kind of return type checks such as existential any that will come under a separate category. We should rename FunctionReturnType to something like FunctionReturnsHeapAllocatedData. This will change the name of the check, corresponding diagnostic override, and the name of the documentation file.

include/swift/AST/Attr.h

artemcm · 2025-10-01T15:57:39Z

@swift-ci test

artemcm · 2025-10-01T16:02:24Z

@swift-ci test

userdocs/diagnostics/perf-hint-return-type-implicit-copy.md

aviralg

LGTM!

DougGregor · 2025-10-01T17:42:52Z

include/swift/AST/DiagnosticGroups.def

 GROUP(UnknownWarningGroup, "unknown-warning-group")
 GROUP(CompilationCaching, "compilation-caching")
 GROUP(WeakMutability, "weak-mutability")
+GROUP(PerfHintReturnTypeImplicitCopy, "perf-hint-return-type-implicit-copy")


This is... very specific. Can we perhaps drop the "PerfHint"/"perf-hint-" prefix here and put them within a "PerformanceHints" group?

The goal here was to allow each check/hint to be enabled/disabled individually. And the idea is the each hint will actually have a small handful of diagnostic varieties, functions and closures being separate diagnostic entires in this PR being an example.

That's fine. You can have an overarching "PerformanceHints" group, and then subgroups for the smaller groups of related diagnostics.

include/swift/AST/Attr.h

include/swift/AST/TypeCheckRequests.h

include/swift/Basic/Features.def

include/swift/AST/DiagnosticsSema.def

include/swift/Option/Options.td

lib/Sema/TypeChecker.cpp

DougGregor · 2025-10-01T18:07:47Z

To be blunt, I don't really like the technical approach here, for a couple of reasons:

We already have a mechanism for opt-in warnings (DefaultIgnore flag + diagnostic groups), and this should be based on that rather than adding an experimental feature (this isn't a language feature).
Adding a global -Wsuppress <group name> is a very big hammer and something that we need to socialize and discuss. It also feels like the wrong level of granularity for suppressing these issues: the warnings should be opt-in for a module (you get that with my suggestion from the previous bullet), so we don't also need some way to make them opt-out again at a module level.
Using a performance-specific attribute (@performanceOverride) for warning suppression also feels like the wrong granularity, albeit in a different manner: it can be reasonable to suppress warnings within a region of code, but the performance hints aren't special in this way. Whether this should be an attribute or a file-level thing, I don't know, but it should apply to all warning groups.

artemcm · 2025-10-01T19:57:20Z

To be blunt, I don't really like the technical approach here, for a couple of reasons:

We already have a mechanism for opt-in warnings (DefaultIgnore flag + diagnostic groups), and this should be based on that rather than adding an experimental feature (this isn't a language feature).

Adding a global -Wsuppress <group name> is a very big hammer and something that we need to socialize and discuss. It also feels like the wrong level of granularity for suppressing these issues: the warnings should be opt-in for a module (you get that with my suggestion from the previous bullet), so we don't also need some way to make them opt-out again at a module level.

Using a performance-specific attribute (@performanceOverride) for warning suppression also feels like the wrong granularity, albeit in a different manner: it can be reasonable to suppress warnings within a region of code, but the performance hints aren't special in this way. Whether this should be an attribute or a file-level thing, I don't know, but it should apply to all warning groups.

@DougGregor thanks for the feedback! I have switched the implementation over to use DefaultIgnore instead of having a suppression flag mechanism, and removed the attribute from this PR so that we can focus on landing the diagnostic logic and discuss any kind of in-source diagnostic suppression mechanism separately.

artemcm · 2025-10-01T21:01:44Z

@swift-ci test

DougGregor

This looks great, thank you!

DougGregor · 2025-10-02T15:14:29Z

include/swift/AST/DiagnosticGroups.def

 GROUP(UnknownWarningGroup, "unknown-warning-group")
 GROUP(CompilationCaching, "compilation-caching")
 GROUP(WeakMutability, "weak-mutability")
+GROUP(PerfHintReturnTypeImplicitCopy, "perf-hint-return-type-implicit-copy")


That's fine. You can have an overarching "PerformanceHints" group, and then subgroups for the smaller groups of related diagnostics.

lib/Option/features.json

DougGregor · 2025-10-02T15:27:09Z

lib/Sema/PerformanceHints.cpp

+  return !ctx.Diags.isIgnoredDiagnostic(diag::perf_hint_closure_returns_array.ID) ||
+         !ctx.Diags.isIgnoredDiagnostic(diag::perf_hint_function_returns_array.ID);


Yeah, this is the best we have right now. At some point (not for this PR!) we should introduce a group-level query for this, which probably means a different underlying data structure.

include/swift/AST/DiagnosticsSema.def

artemcm · 2025-10-02T16:28:25Z

@swift-ci smoke test

… and dictionary type This check will run on each type-checked primary input of the current compilation and emit a warning diagnostic for all discovered occurences of this code pattern when the performance hint diagnostic is enabled

artemcm · 2025-10-02T17:31:35Z

@swift-ci smoke test

aviralg reviewed Sep 30, 2025

View reviewed changes

include/swift/AST/Attr.h Outdated Show resolved Hide resolved

artemcm force-pushed the SPA-Init branch 2 times, most recently from 7796cb7 to 89cd4ba Compare October 1, 2025 15:39

artemcm marked this pull request as ready for review October 1, 2025 15:57

artemcm requested review from hborla, slavapestov, xedin, xymus, bnbarham, CodaFi, DougGregor, hamishknight, rintaro and tshortli as code owners October 1, 2025 15:57

artemcm force-pushed the SPA-Init branch from 89cd4ba to db8d2e0 Compare October 1, 2025 16:02

aviralg reviewed Oct 1, 2025

View reviewed changes

userdocs/diagnostics/perf-hint-return-type-implicit-copy.md Outdated Show resolved Hide resolved

aviralg approved these changes Oct 1, 2025

View reviewed changes

DougGregor reviewed Oct 1, 2025

View reviewed changes

artemcm force-pushed the SPA-Init branch from db8d2e0 to 3ff320c Compare October 1, 2025 17:51

artemcm force-pushed the SPA-Init branch 3 times, most recently from d053a90 to 6cb0646 Compare October 1, 2025 19:56

artemcm force-pushed the SPA-Init branch 2 times, most recently from 7050c7b to d08816a Compare October 1, 2025 21:01

DougGregor approved these changes Oct 2, 2025

View reviewed changes

artemcm force-pushed the SPA-Init branch from d08816a to 4bf3efc Compare October 2, 2025 16:28

artemcm enabled auto-merge October 2, 2025 16:28

artemcm force-pushed the SPA-Init branch from 4bf3efc to 7c74890 Compare October 2, 2025 17:31

artemcm merged commit 96ee032 into swiftlang:main Oct 2, 2025
3 checks passed

artemcm deleted the SPA-Init branch October 3, 2025 15:56

		return !ctx.Diags.isIgnoredDiagnostic(diag::perf_hint_closure_returns_array.ID) \|\|
		!ctx.Diags.isIgnoredDiagnostic(diag::perf_hint_function_returns_array.ID);

[Performance Hints] Add simple check for returning of values of array and dictionary type #84578

[Performance Hints] Add simple check for returning of values of array and dictionary type #84578

Uh oh!

Conversation

artemcm commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviralg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artemcm commented Oct 1, 2025

Uh oh!

artemcm commented Oct 1, 2025

Uh oh!

Uh oh!

aviralg left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

artemcm Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

DougGregor Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DougGregor commented Oct 1, 2025

Uh oh!

artemcm commented Oct 1, 2025

Uh oh!

artemcm commented Oct 1, 2025

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DougGregor Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artemcm commented Oct 2, 2025

Uh oh!

artemcm commented Oct 2, 2025

Uh oh!

Uh oh!

Uh oh!

artemcm commented Sep 30, 2025 •

edited

Loading