Allow specific overloads to be specified in ForbiddenMethod

[fourls]

Prerequisites

• [X] This improvement has not already been suggested.
• [X] This improvement should not be implemented as a separate rule.

Rule to improve

ForbiddenMethod

Improvement description

Custom rules based off the ForbiddenMethod template rule should be able to optionally specify which overloads should be forbidden.

This could look like optionally including an argument list of type names to the blacklist, e.g. MyUnit.MyType.MyProc(Integer, string).

• These type names could have to be an exact string match for the types as they are in the method declaration, which would ease name resolution issues.
• Explicitly specifying an overload with no parameters is as easy as adding an empty argument list ().

Rationale

It's currently impossible to specify some overloads as forbidden, which is a fairly common use case (e.g. an outdated or suboptimal version of the method that is only maintained for compatibility). A great example of where SonarDelphi already acknowledges this is in the rule forbidding the Single overloads of math functions.

[cirras]

Background

In SonarDelphi, Type instances derive their exact identity from a string image. Type::is really just performs a string match against the other type's image.

Naive solution

So naturally you would think "Great, then just let the user specify the type image in an argument list," right? You'd get something like Foo(System.String, System.Classes.TStream), and everything would be awesome.

Challenges

• Types don't always have names in Delphi.
  • The analyzer tries to derive type images from the fully-qualified name of the type declaration, but this isn't always possible.
  • This leads to ugly type images like array of System.Byte <OPEN>
• Synthesized type images would become part of the user-facing API for ForbiddenNameRule
  • We'd have to be extra considerate of type image changes, so as not to break people's custom rules.

Considerations

• For method parameters in particular, there are only a subset of valid types references.
  • For example, the only "unnamed" array types allowed are "open" arrays.
• We could potentially sanitize/generalize some type images or even sidestep them altogether for comparison purposes.

tl;dr Representing types as strings in Delphi is challenging compared to a language like Java, where everything is either a primitive or a fully-qualified class name - so how should we do it?

[fourls]

The easiest solution would be not to do any type resolution at all, and check the user-provided signature against the exact text of the declaration. For example, for the following:

uses System.Classes;

procedure Foo(const A: string, B: array of Byte, C: TStringList);

The user-provided signature must then be Foo(string, array of Byte, TStringList) (case- and whitespace-insensitive).

This is the approach I did in pasdoc/pasdoc#161 for reading user-provided signatures (although this was the only real choice as PasDoc does not perform semantic analysis). It has some cons:

• It is possible to have two overloads with the same simple signature if they refer to different types. In this case, both overloads would be picked up. I'm inclined to say "you're holding it wrong" here and not worry about this situation.
• Changing the routine signature in a non-semantically important way (e.g. replacing a type with a weak alias, or fully qualifying a type) would require the rule blacklist to be changed.

It also has some notable pros:

• It is simple to understand.
• Practically speaking you will never get clashes.
• It is supportive of type aliases.
• It is significantly less complex than a full type resolution approach, and so is more likely to work as intended.

[cirras]

The easiest solution would be not to do any type resolution at all, and check the user-provided signature against the exact text of the declaration. For example, for the following:

uses System.Classes;

procedure Foo(const A: string, B: array of Byte, C: TStringList);

The user-provided signature must then be Foo(string, array of Byte, TStringList) (case- and whitespace-insensitive).

This is the approach I did in pasdoc/pasdoc#161 for reading user-provided signatures (although this was the only real choice as PasDoc does not perform semantic analysis). It has some cons:

• It is possible to have two overloads with the same simple signature if they refer to different types. In this case, both overloads would be picked up. I'm inclined to say "you're holding it wrong" here and not worry about this situation.
• Changing the routine signature in a non-semantically important way (e.g. replacing a type with a weak alias, or fully qualifying a type) would require the rule blacklist to be changed.

It also has some notable pros:

• It is simple to understand.
• Practically speaking you will never get clashes.
• It is supportive of type aliases.
• It is significantly less complex than a full type resolution approach, and so is more likely to work as intended.

I think that I like this solution, and here's why:

• It's simple to implement
• It's intuitive for the user (copy-paste)
• It lets us sidestep all of the aforementioned issues with type images

Sounds good to me.

[cirras]

I've been thinking about this and an obvious problem occurred to me in hindsight.

From the call-site, the analysis rule would only have symbol and type information from the RoutineNameDeclaration in the symbol table. The "raw" signature and exact type strings would be long gone.

More thinking needed on this one.