[202x] Enable auto return types - #8901
Open
Chris B (llvm-beanz) wants to merge 7 commits into
Open
Chris B (llvm-beanz) wants to merge 7 commits into
Chris B (llvm-beanz) wants to merge 7 commits into
Conversation
This PR enables return type deduction for normal functions following C++ 14's use of `auto` as the return type in normal function prototype syntax. The HLSL proposal is in progress here: https://hlsl-tc57.github.io/tc57/proposal/0010 While this proposal is still in Refinement, so this is getting a bit ahead of the standard committee it seems like a highly likely feature that brings a lot of value for a fairly small set of changes. Assisted-by: GitHub Copilot ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-extension-war ning.hlsl ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-type.hlsl
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
HLSL-specific deduction restrictions are bypassed, and important test and release-note coverage is missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables C++14-style auto return-type deduction for HLSL functions across semantic analysis and code generation.
Changes:
- Enables deduction during declaration, return analysis, and function use.
- Adds DXIL and SPIR-V validation.
- Adds diagnostics and legacy-version warning tests.
File summaries
| File | Description |
|---|---|
tools/clang/lib/Sema/SemaType.cpp |
Permits deduced function return types in HLSL. |
tools/clang/lib/Sema/SemaStmt.cpp |
Deduces types from return statements. |
tools/clang/lib/Sema/SemaExpr.cpp |
Handles uses of functions with undeduced returns. |
tools/clang/lib/Sema/SemaDecl.cpp |
Handles declarations, templates, and implicit void. |
tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-type.hlsl |
Tests standard deduction and DXIL output. |
tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-extension-warning.hlsl |
Tests legacy-mode warnings. |
tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl |
Tests invalid deduction cases. |
tools/clang/test/CodeGenSPIRV/fn.auto.return.hlsl |
Tests SPIR-V return types. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+7488
to
7491
| if ((getLangOpts().CPlusPlus14 || getLangOpts().HLSL) && // HLSL Change | ||
| (NewFD->isDependentContext() || | ||
| (isFriend && CurContext->isDependentContext())) && | ||
| NewFD->getReturnType()->isUndeducedType()) { |
Comment on lines
3756
to
+3758
| if (D.getDeclSpec().containsPlaceholderType() && | ||
| !FTI.hasTrailingReturnType() && chunkIndex == 0 && | ||
| !S.getLangOpts().CPlusPlus14) { | ||
| !(S.getLangOpts().CPlusPlus14 || S.getLangOpts().HLSL)) { |
Collaborator
|
Do we want to disallow auto for entry-point functions? |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Dynamic descriptor-heap returns bypass existing auto restrictions, and release-note coverage is missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tools/clang/lib/Sema/SemaStmt.cpp:3080
- The new return-type path omits the dynamic descriptor-heap check used for
autovariables inSemaDecl.cpp:6421-6433and9047-9053. Those heap expressions haveHLSLDynamicResourceAttr, notHLSLNonAutoDeducibleAttr, soIsTypeDeducibleWithAutoaccepts them and a function can deduce an internal dynamic resource/sampler return type even thoughauto x = ResourceDescriptorHeap[i]is rejected. Apply the same heap-expression check here before generic type classification and add resource/sampler return tests.
if (getLangOpts().HLSL && !Deduced->isDependentType() &&
!hlsl::IsTypeDeducibleWithAuto(*this, Deduced)) {
Diag(RetExpr->getExprLoc(), diag::err_hlsl_auto_undeducible_type)
<< Deduced;
tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl:29
- The comment announces initializer-list coverage, but no initializer-list return test follows it, so the existing
err_auto_fn_return_init_listpath is not exercised in HLSL mode. Add the omittedautofunction withreturn { 1, 2, 3 };and verifycannot deduce return type from initializer list.
// Returning an initializer list as the deduced return value is not allowed.
// (Initializer lists in return statements are also unsupported in HLSL more
// broadly, but the auto-return-type case is still flagged early.)
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+2661
to
+2665
| // HLSL Change Begin - HLSL supports 'auto' as a function declarator return | ||
| // type with C++14-style deduction; skip this check for functions. | ||
| if (ContainsPlaceholderType && | ||
| (!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator())) { | ||
| (!(SemaRef.getLangOpts().CPlusPlus11 || SemaRef.getLangOpts().HLSL) || | ||
| !D.isFunctionDeclarator())) { |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Auto returns currently bypass descriptor-heap restrictions, and release-note and test coverage gaps remain.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
tools/clang/lib/Sema/SemaStmt.cpp:3081
- This check misses the separate descriptor-heap rule used for
autovariables..Resource/.SamplercarryHLSLDynamicResourceAttr, notHLSLNonAutoDeducibleAttr, soIsTypeDeducibleWithAutoreturns true and anautofunction can expose the otherwise-forbidden placeholder type withreturn ResourceDescriptorHeap[i](orSamplerDescriptorHeap[i]). Apply the equivalent dynamic-heap expression check here, ideally via a shared helper, and add return-type cases alongsideauto-no-descriptor-heap.hlsl.
tools/clang/lib/Sema/SemaType.cpp:2665
- This is a user-visible HLSL language feature, so the repository's release-note policy requires an entry in
docs/ReleaseNotes.mdunderUpcoming Release/HLSL Language. Please add that entry, or identify the related PR that will provide shared release-note coverage.
// HLSL Change Begin - HLSL supports 'auto' as a function declarator return
// type with C++14-style deduction; skip this check for functions.
if (ContainsPlaceholderType &&
(!(SemaRef.getLangOpts().CPlusPlus11 || SemaRef.getLangOpts().HLSL) ||
!D.isFunctionDeclarator())) {
tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl:29
- This comment describes an initializer-list diagnostic, but the file contains no initializer-list return case after it, so that behavior is not actually tested. Add an
autofunction returning a braced initializer with the expected diagnostic (or remove the claim if it is intentionally out of scope).
// Returning an initializer list as the deduced return value is not allowed.
// (Initializer lists in return statements are also unsupported in HLSL more
// broadly, but the auto-return-type case is still flagged early.)
tools/clang/lib/Sema/SemaDecl.cpp:7490
- The new dependent-context path is not exercised by the added tests:
auto-template.hlslonly has templates with explicit return types. Add an HLSL template such astemplate<typename T> auto Identity(T value) { return value; }and instantiate it for multiple types; also cover deferred rejection of a non-auto-deducible type so the instantiation-time check is verified.
if ((getLangOpts().CPlusPlus14 || getLangOpts().HLSL) && // HLSL Change
(NewFD->isDependentContext() ||
(isFriend && CurContext->isDependentContext())) &&
NewFD->getReturnType()->isUndeducedType()) {
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-undeducible-t ypes.hlsl ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-errors.hlsl ../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-undeducible-t ypes.hlsl
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Release-note coverage, template-instantiation tests, and updated attribute documentation are still needed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tools/clang/lib/Sema/SemaType.cpp:3758
- This is a user-visible HLSL language feature, but the PR does not add the release note required for new language features by
CONTRIBUTING.md:115-134. Please add a concise entry underdocs/ReleaseNotes.md→Upcoming Release→HLSL Language(or point to the related PR that will provide the shared entry).
// HLSL Change Begin - HLSL supports C++14-style deduced return types.
if (D.getDeclSpec().containsPlaceholderType() &&
!FTI.hasTrailingReturnType() && chunkIndex == 0 &&
!(S.getLangOpts().CPlusPlus14 || S.getLangOpts().HLSL)) {
tools/clang/lib/Sema/SemaDecl.cpp:7490
- This explicitly enables deduced returns for dependent functions, but none of the added tests exercises
template<typename T> auto f(T); the existingauto-template.hlslonly usesautofor local variables in templates. Please cover successful instantiation (ideally for multiple types) and an instantiation whose return type is non-auto-deducible, since this branch defers deduction and validation until instantiation.
if ((getLangOpts().CPlusPlus14 || getLangOpts().HLSL) && // HLSL Change
(NewFD->isDependentContext() ||
(isFriend && CurContext->isDependentContext())) &&
NewFD->getReturnType()->isUndeducedType()) {
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
| CXXRecordDecl *recordDecl = typeDeclBuilder.getRecordDecl(); | ||
| recordDecl->addAttr( | ||
| HLSLDynamicResourceAttr::CreateImplicit(context, bSampler)); | ||
| recordDecl->addAttr(HLSLNonAutoDeducibleAttr::CreateImplicit(context)); |
../tools/clang/test/HLSLFileCheckLit/hlsl/auto/auto-return-undeducible-t ypes.hlsl
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The user-visible language feature needs a release note and template return-deduction coverage.
Review details
Suppressed comments (2)
tools/clang/lib/Sema/SemaType.cpp:2665
- This is a user-visible HLSL language feature, but the PR does not add the release note required by
CONTRIBUTING.mdfor significant language changes. Please add a one-sentence entry underdocs/ReleaseNotes.md→Upcoming Release→HLSL Language, or point to the related PR that will provide the shared release-note coverage.
(!(SemaRef.getLangOpts().CPlusPlus11 || SemaRef.getLangOpts().HLSL) ||
!D.isFunctionDeclarator())) {
tools/clang/lib/Sema/SemaDecl.cpp:7490
- This branch explicitly enables deduced returns in dependent function contexts, but the new tests cover only non-template functions; the existing
auto-template.hlsluses explicit return types. Please add coverage for an instantiatedtemplate<typename T> autofunction (including at least two concrete return types), so regressions in the template-instantiation deduction path are detected.
if ((getLangOpts().CPlusPlus14 || getLangOpts().HLSL) && // HLSL Change
(NewFD->isDependentContext() ||
(isFriend && CurContext->isDependentContext())) &&
NewFD->getReturnType()->isUndeducedType()) {
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Release-note coverage and tests for the newly enabled dependent-function path are missing.
Review details
Suppressed comments (2)
tools/clang/lib/Sema/SemaDecl.cpp:7490
- This branch also enables deduced return types for dependent functions, but none of the added tests declares a function template with an
autoreturn type; the existingauto-template.hlslonly usesautofor local variables. Please cover at least successful instantiations with distinct deduced types and an instantiation whose return type is non-auto-deducible, so this newly enabled path and its deferred check cannot regress.
if ((getLangOpts().CPlusPlus14 || getLangOpts().HLSL) && // HLSL Change
(NewFD->isDependentContext() ||
(isFriend && CurContext->isDependentContext())) &&
NewFD->getReturnType()->isUndeducedType()) {
tools/clang/lib/Sema/SemaType.cpp:2665
- This enables a user-visible HLSL language feature, but the PR does not update
docs/ReleaseNotes.md. The repository policy inCONTRIBUTING.md:117-128explicitly lists new language features as significant compiler changes requiring a release note. Please add a single-sentence entry underUpcoming Release→HLSL Language, or point to the related PR that will provide shared release-note coverage.
(!(SemaRef.getLangOpts().CPlusPlus11 || SemaRef.getLangOpts().HLSL) ||
!D.isFunctionDeclarator())) {
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
This PR enables return type deduction for normal functions following C++ 14's use of
autoas the return type in normal function prototype syntax.The HLSL proposal is in progress here:
https://hlsl-tc57.github.io/tc57/proposal/0010
While this proposal is still in Refinement, so this is getting a bit ahead of the standard committee it seems like a highly likely feature that brings a lot of value for a fairly small set of changes.
Resolves #8903
Assisted-by: GitHub Copilot