From a4345725a083651c16795d27fedd769f2d7e55ae Mon Sep 17 00:00:00 2001 From: Yong He Date: Mon, 16 Jun 2025 20:37:27 -0700 Subject: Require `override` keyword for overriding default interface methods. (#7458) * Require `override` keyword for overriding default interface methods. * Update doc. * Fix test. --- source/slang/slang-ast-modifier.h | 15 ++++++++ source/slang/slang-check-decl.cpp | 64 +++++++++++++++++++++++++++++++++++ source/slang/slang-check-impl.h | 5 +++ source/slang/slang-check-modifier.cpp | 2 ++ source/slang/slang-diagnostic-defs.h | 11 ++++++ source/slang/slang-parser.cpp | 1 + 6 files changed, 98 insertions(+) (limited to 'source') diff --git a/source/slang/slang-ast-modifier.h b/source/slang/slang-ast-modifier.h index 4f6a08e4f..ca023a19b 100644 --- a/source/slang/slang-ast-modifier.h +++ b/source/slang/slang-ast-modifier.h @@ -66,6 +66,21 @@ class InternalModifier : public VisibilityModifier FIDDLE(...) }; +FIDDLE() +class OverrideModifier : public Modifier +{ + FIDDLE(...) +}; + +// Marks that a decl is verified to be overriding another decl defined in a base type. +FIDDLE() +class IsOverridingModifier : public Modifier +{ + FIDDLE(...) + + FIDDLE() Decl* overridedDecl = nullptr; +}; + FIDDLE() class RequireModifier : public Modifier { diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp index 8471acb0a..23458c94a 100644 --- a/source/slang/slang-check-decl.cpp +++ b/source/slang/slang-check-decl.cpp @@ -5055,6 +5055,40 @@ HasInterfaceDefaultImplModifier* hasDefaultImpl(DeclRef declRef) return nullptr; } +void SemanticsVisitor::markOverridingDecl( + ConformanceCheckingContext* context, + Decl* memberDecl, + DeclRef requiredMemberDeclRef) +{ + if (!memberDecl->isChildOf(context->parentDecl)) + { + // If the member being checked isn't the child of the container decl containing + // the inheritance decl that triggers the conformance check, don't modify/diagnose + // anything since it should have already been diagnosed when checking its parent. + // This can happen when we check for things like: + // extension float : IComparable{} + // where the method used to satisfy IComparable comes from outside the extension decl. + // we don't want to diagnose or check anything about the found memberDecl that doesn't + // belong to this extension decl (context->parentDecl). + // + return; + } + + if (hasDefaultImpl(requiredMemberDeclRef)) + { + memberDecl = maybeGetInner(memberDecl); + // If the required member has a default implementation, + // we need to make sure the member we found is marked as 'override'. + if (!memberDecl->hasModifier()) + { + getSink()->diagnose(memberDecl, Diagnostics::missingOverride); + } + } + auto overridingModifier = m_astBuilder->create(); + overridingModifier->overridedDecl = requiredMemberDeclRef.getDecl(); + addModifier(memberDecl, overridingModifier); +} + bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( ConformanceCheckingContext* context, LookupResult const& lookupResult, @@ -5345,6 +5379,8 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( return false; } } + + markOverridingDecl(context, callee.getDecl(), requiredMemberDeclRef); } } } @@ -7232,6 +7268,7 @@ bool SemanticsVisitor::findWitnessForInterfaceRequirement( QualifiedDeclPath(requiredMemberDeclRef)); return false; } + markOverridingDecl(context, member.declRef.getDecl(), requiredMemberDeclRef); return true; } } @@ -7738,6 +7775,33 @@ void SemanticsVisitor::checkAggTypeConformance(AggTypeDecl* decl) // only the types that are affected by these interface decls. // astBuilder->incrementEpoch(); + + // For all members marked as `override`, we need to ensure they are actually + // overriding something. + for (auto member : decl->getMembers()) + { + auto innerMember = maybeGetInner(member); + bool hasOverride = false; + bool isOverriding = false; + for (auto modifier : innerMember->modifiers) + { + if (as(modifier)) + { + hasOverride = true; + } + else if (as(modifier)) + { + isOverriding = true; + } + } + if (hasOverride && !isOverriding) + { + getSink()->diagnose( + innerMember, + Diagnostics::overrideModifierNotOverridingBaseDecl, + innerMember); + } + } } } diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h index be6b1c2fc..365f21b1c 100644 --- a/source/slang/slang-check-impl.h +++ b/source/slang/slang-check-impl.h @@ -1892,6 +1892,11 @@ public: DeclRef requirement, DeclRef method); + void markOverridingDecl( + ConformanceCheckingContext* context, + Decl* memberDecl, + DeclRef requiredMemberDeclRef); + /// Attempt to synthesize a method that can satisfy `requiredMemberDeclRef` using /// `lookupResult`. /// diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp index 1d38fa802..2a4707db5 100644 --- a/source/slang/slang-check-modifier.cpp +++ b/source/slang/slang-check-modifier.cpp @@ -1529,6 +1529,8 @@ bool isModifierAllowedOnDecl(bool isGLSLInput, ASTNodeType modifierType, Decl* d return isGlobalDecl(decl) || isEffectivelyStatic(decl); case ASTNodeType::DynModifier: return as(decl) || as(decl) || as(decl); + case ASTNodeType::OverrideModifier: + return as(decl) && as(getParentDecl(decl)); default: return true; } diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h index 4f0edf53b..64b47ece8 100644 --- a/source/slang/slang-diagnostic-defs.h +++ b/source/slang/slang-diagnostic-defs.h @@ -1731,6 +1731,17 @@ DIAGNOSTIC( invalidExtensionOnInterface, "cannot extend interface type '$0'. consider using a generic extension: `extension T " "{...}`.") +DIAGNOSTIC( + 30853, + Error, + missingOverride, + "missing 'override' keyword for methods that overrides the default implementation in the " + "interface.") +DIAGNOSTIC( + 30854, + Error, + overrideModifierNotOverridingBaseDecl, + "'$0' marked as 'override' is not overriding any base declarations.") // 309xx: subscripts DIAGNOSTIC( diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp index 6d6341e4e..9727140f3 100644 --- a/source/slang/slang-parser.cpp +++ b/source/slang/slang-parser.cpp @@ -9467,6 +9467,7 @@ static const SyntaxParseInfo g_parseSyntaxEntries[] = { _makeParseModifier("writeonly", parseWriteonlyModifier), _makeParseModifier("export", getSyntaxClass()), _makeParseModifier("dynamic_uniform", getSyntaxClass()), + _makeParseModifier("override", getSyntaxClass()), // Modifiers for geometry shader input _makeParseModifier("point", getSyntaxClass()), -- cgit v1.2.3