Improve error reporting for duplicate-definition functions.

Previously, we would err on the side of showing the "other" function
instead of showing the user's function. However, when the "other"
function is from a module, it can leak a `$genType` or a `$pure` into
the error message, which might be confusing for users. Now, we will
err on the side of showing the user's function more often.

This change also lets `FunctionDefinition::description()` return
more accurate results. Previously, we were hiding
`sk_has_side_effects`/`$pure` to make error messages look nicer,
but that meant that a FunctionDefinition's description() was not
round-trip-safe.

Change-Id: Ie45568f1ff39b075fdd002481351cddf26c507d1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/584399
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

resources/sksl/errors/DuplicateFunction.rts

@@ -2,5 +2,5 @@ void func() {}
 void func() {}
 
 /*%%*
-duplicate definition of void func()
+duplicate definition of 'void func()'
 *%%*/

resources/sksl/errors/Ossfuzz38140.sksl

@@ -7,5 +7,5 @@ half4 main(half4 src, half4 dst) {
 }
 
 /*%%*
-duplicate definition of half4 blend_src_over(half4 src, half4 dst)
+duplicate definition of 'half4 blend_src_over(half4 src, half4 dst)'
 *%%*/

resources/sksl/errors/OverloadedBuiltin.sksl

@@ -17,10 +17,10 @@ float pow(float2 x, float y) { return 0; /* allowed */ }
 float pow(half x, float y) { return 0; /* allowed */ }
 
 /*%%*
-duplicate definition of $genHType fma($genHType a, $genHType b, $genHType c)
-duplicate definition of $genHType sin($genHType angle)
-duplicate definition of $genType sin($genType angle)
-duplicate definition of $genHType cos($genHType angle)
-functions 'float2 cos(half2 a)' and '$genHType cos($genHType angle)' differ only in return type
-duplicate definition of $genType pow($genType x, $genType y)
+duplicate definition of 'half fma(half a, half b, half c)'
+duplicate definition of 'half2 sin(half2 a)'
+duplicate definition of 'float3 sin(float3 a)'
+duplicate definition of 'half cos(half2 a)'
+functions 'float2 cos(half2 a)' and '$pure $genHType cos($genHType angle)' differ only in return type
+duplicate definition of 'float pow(float x, float y)'
 *%%*/

src/sksl/ir/SkSLFunctionDeclaration.cpp

@@ -378,11 +378,27 @@ static bool parameters_match(const std::vector<std::unique_ptr<Variable>>& param
 static bool find_existing_declaration(const Context& context,
                                       SymbolTable& symbols,
                                       Position pos,
+                                      const Modifiers* modifiers,
                                       std::string_view name,
                                       std::vector<std::unique_ptr<Variable>>& parameters,
                                       Position returnTypePos,
                                       const Type* returnType,
                                       const FunctionDeclaration** outExistingDecl) {
+    auto invalidDeclDescription = [&]() -> std::string {
+        std::vector<const Variable*> paramPtrs;
+        paramPtrs.reserve(parameters.size());
+        for (std::unique_ptr<Variable>& param : parameters) {
+            paramPtrs.push_back(param.get());
+        }
+        return FunctionDeclaration(pos,
+                                   modifiers,
+                                   name,
+                                   std::move(paramPtrs),
+                                   returnType,
+                                   context.fConfig->fIsBuiltinCode)
+                .description();
+    };
+
     ErrorReporter& errors = *context.fErrors;
     const Symbol* entry = symbols[name];
     *outExistingDecl = nullptr;
@@ -398,31 +414,21 @@ static bool find_existing_declaration(const Context& context,
                 continue;
             }
             if (!type_generically_matches(*returnType, other->returnType())) {
-                std::vector<const Variable*> paramPtrs;
-                paramPtrs.reserve(parameters.size());
-                for (std::unique_ptr<Variable>& param : parameters) {
-                    paramPtrs.push_back(param.get());
-                }
-                FunctionDeclaration invalidDecl(pos,
-                                                &other->modifiers(),
-                                                name,
-                                                std::move(paramPtrs),
-                                                returnType,
-                                                context.fConfig->fIsBuiltinCode);
                 errors.error(returnTypePos,
-                             "functions '" + invalidDecl.description() + "' and '" +
+                             "functions '" + invalidDeclDescription() + "' and '" +
                              other->description() + "' differ only in return type");
                 return false;
             }
             for (size_t i = 0; i < parameters.size(); i++) {
                 if (parameters[i]->modifiers() != other->parameters()[i]->modifiers()) {
-                    errors.error(parameters[i]->fPosition, "modifiers on parameter " +
-                            std::to_string(i + 1) + " differ between declaration and definition");
+                    errors.error(parameters[i]->fPosition,
+                                 "modifiers on parameter " + std::to_string(i + 1) +
+                                 " differ between declaration and definition");
                     return false;
                 }
             }
-            if (other->definition() || other->isBuiltin()) {
-                errors.error(pos, "duplicate definition of " + other->description());
+            if (*modifiers != other->modifiers() || other->definition() || other->isBuiltin()) {
+                errors.error(pos, "duplicate definition of '" + invalidDeclDescription() + "'");
                 return false;
             }
             *outExistingDecl = other;
@@ -467,8 +473,8 @@ const FunctionDeclaration* FunctionDeclaration::Convert(
         !check_return_type(context, returnTypePos, *returnType) ||
         !check_parameters(context, parameters, isMain) ||
         (isMain && !check_main_signature(context, pos, *returnType, parameters)) ||
-        !find_existing_declaration(context, symbols, pos, name, parameters, returnTypePos,
-                                   returnType, &decl)) {
+        !find_existing_declaration(context, symbols, pos, modifiers, name, parameters,
+                                   returnTypePos, returnType, &decl)) {
         return nullptr;
     }
     std::vector<const Variable*> finalParameters;
@@ -511,10 +517,7 @@ std::string FunctionDeclaration::mangledName() const {
 }
 
 std::string FunctionDeclaration::description() const {
-    // We don't want to show `$pure` and `$es3` on function descriptions, even if it's true.
     int modifierFlags = this->modifiers().fFlags;
-    modifierFlags &= ~(Modifiers::kPure_Flag | Modifiers::kES3_Flag);
-
     std::string result =
             (modifierFlags ? Modifiers::DescribeFlags(modifierFlags) + " " : std::string()) +
             this->returnType().displayName() + " " + std::string(this->name()) + "(";

tests/sksl/errors/DuplicateFunction.glsl

@@ -1,6 +1,6 @@
 ### Compilation failed:
 
-error: 2: duplicate definition of void func()
+error: 2: duplicate definition of 'void func()'
 void func() {}
 ^^^^^^^^^^^
 1 error

tests/sksl/errors/Ossfuzz38140.glsl

@@ -1,6 +1,6 @@
 ### Compilation failed:
 
-error: 1: duplicate definition of half4 blend_src_over(half4 src, half4 dst)
+error: 1: duplicate definition of 'half4 blend_src_over(half4 src, half4 dst)'
 half4 blend_src_over(half4 src, half4 dst) {
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 error: 2: unknown identifier 'src'

tests/sksl/errors/OverloadedBuiltin.glsl

@@ -1,21 +1,21 @@
 ### Compilation failed:
 
-error: 4: duplicate definition of $genHType fma($genHType a, $genHType b, $genHType c)
+error: 4: duplicate definition of 'half fma(half a, half b, half c)'
 half fma(half a, half b, half c) { return 0; /* error: overloads a builtin */ }
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-error: 7: duplicate definition of $genHType sin($genHType angle)
+error: 7: duplicate definition of 'half2 sin(half2 a)'
 half2 sin(half2 a) { return half2(0); /* error: overloads a builtin */ }
 ^^^^^^^^^^^^^^^^^^
-error: 8: duplicate definition of $genType sin($genType angle)
+error: 8: duplicate definition of 'float3 sin(float3 a)'
 float3 sin(float3 a) { return float3(0); /* error: overloads a builtin */ }
 ^^^^^^^^^^^^^^^^^^^^
-error: 12: duplicate definition of $genHType cos($genHType angle)
+error: 12: duplicate definition of 'half cos(half2 a)'
 half cos(half2 a) { return 0; /* error: overloads a builtin (despite return type mismatch) */ }
 ^^^^^^^^^^^^^^^^^
-error: 13: functions 'float2 cos(half2 a)' and '$genHType cos($genHType angle)' differ only in return type
+error: 13: functions 'float2 cos(half2 a)' and '$pure $genHType cos($genHType angle)' differ only in return type
 float2 cos(half2 a) { return 0; /* error: overloads a builtin (despite return type mismatch) */ }
 ^^^^^^^^^^^^^^^^^^^
-error: 15: duplicate definition of $genType pow($genType x, $genType y)
+error: 15: duplicate definition of 'float pow(float x, float y)'
 float pow(float x, float y) { return 0; /* error: overloads a builtin */ }
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 6 errors