False positive: rule community-delphi:UnusedConstant

[simonegirlanda]

Prerequisites

• [x] This bug is in SonarDelphi, not SonarQube or my Delphi code.
• [x] This bug has not already been reported.

SonarDelphi version

1.15.0

Delphi version

10.4

SonarQube version

10.7

Issue description

We are having hundreds of this false positive, on constants, but also on other elements. The problem isn't easy to replicate, some time the problem disappear, apparently without intentional changes. I was able to isolate the problem in the attached unit.

In this specific case, constant declared in line 7, is used in line 50.

Steps to reproduce

execute code analysis on the attached unit.

Minimal Delphi code exhibiting the issue

unit SistemaFrm;

interface

const CM_CREATE_RECUR = WM_USER + 10000;

type TcxCheckGroupAccess = class(TcxCheckGroup); TSistemaForm = class(TAnagForm, IDckExplorerChildForm)

procedure   AggiuntiSingoliEventiRipetitivi( AEvent: TcxSchedulerControlEvent);

end;

var SistemaForm : TSistemaForm; aNewEvent : Boolean;

implementation

{$R *.dfm}

procedure TSistemaForm.AggiuntiSingoliEventiRipetitivi( AEvent: TcxSchedulerControlEvent); var afield : Variant; begin if not Assigned(AEvent) then // event deleted or not created Exit;

if AEvent <> Nil then try if True then begin

  inherited;

  // Aggiorno la descrizione dell'evento con il codice del punto di prelievo
  //afield := aEvent.GetCustomFieldValueByName('cal_ptoPrelievo');
  if Not(Varisnull(Afield))
     then aEvent.Caption := aField
     else ;
  if AEvent.IsRecurring and (AEvent.EventType = etPattern) then Begin// Is the parent event
    // Prima inserisco l'evento padre ... e poi inserisco tutte le sue occorrenze
    aEvent.Caption := aField;
    //aEvent.AllDayEvent := False;
    PostMessage( Self.Handle, CM_CREATE_RECUR, 0, AEvent.ID);
  end
end;

Finally end; end;

begin registerclass(TSistemaForm); end.

[cirras]

Hi @simonegirlanda,

My thinking is that one of the arguments to PostMessage has an unresolved type and is causing name resolution failures for the whole invocation.

Some questions:

• does your scan complete without warnings/errors?
• if you run your scan with debug logging enabled (-X flag) are there any unresolved units (with an X next to them in the tree)?

[simonegirlanda]

Ciao @Cirras thank you for your answer

My thinking is that one of the arguments to PostMessage has an unresolved type and is causing name resolution failures for the whole invocation.

ok thanks, I'll check.

Scan was made with -X, apparently no related errors/warnings, just this one:

18:04:13.834 WARN  Missing blame information for the following files:
18:04:13.835 WARN    * AreaEnteQ/FORMS/SistemaFrm.pas
18:04:13.835 WARN  This may lead to missing/broken features in SonarQube

[simonegirlanda]

sonar.log

[simonegirlanda]

@Cirras I confirm that the false positive is related to the way the constant is used.

adding this instruction:

variable := CM_CREATE_RECUR

solve the issue. after removing the instruction the error is issued again.

[cirras]

Hi @simonegirlanda,

Looking at your scan log made me realize that your "minimal example" has no uses clauses and therefore isn't valid/compilable code.

It's pretty much guaranteed that things will go wrong in the analysis if you're missing necessary imports.

Please:

• provide a compilable example
• provide a new scan log

[simonegirlanda]

Hi @Cirras the "minimal example" supplied is an extract from our original source code that is valid/compilable and where we receive the same false positive.

I will produce a new compilable example with a new scan.

Thank you for your support.

[cirras]

Hi @simonegirlanda,

Any luck with a new reproduction case?

[simonegirlanda]

No. :-(

If I write a minimal compilable example, I have no issue. We got these kind of issues only in our original complex code (both compilable and not compilable) with tons of uses. No errors or warning in the logs.

These are very annoying issues, we will spend some more time next week to try to find a root cause.

[EduardoVaz06]

I've also been seeing lots of false positives, both in UnusedConstantCheck and UnusedLocalVariable.

One example is this one (I simply added new lines in the already existant test)

As it seems the false positive happens when the constant/variable is used as a parameter in a method call or similar.

[cirras]

Hi @EduardoVaz06,

As in the case above, we need the reproduction case to be compilable code. The analyser won't be able to resolve the Canvas.TextRec method in your test because you haven't declared a Canvas variable.

If the method call cannot be resolved, then none of the arguments will be resolved or recognised as used.

There are different reasons why an invocation might not be resolved. A common one is that the sonar project has been configured incorrectly, and some units/imports are unavailable to the analyzer as a result. This causes name resolution failures.

Bugs in the analyzer are also possible - we've fixed several in this upcoming release alone. Name resolution (and invocation/overload resolution) is complicated and the exact rules are not documented. If you believe that your false positives are the result of a genuine bug in the analysis engine and not a misconfiguration, we really need to see a minimal reproduction case that compiles.

[cirras]

I've raised #377 to improve the situation (in all rules - including UnusedConstant and UnusedLocalVariable) when symbol information is incomplete.

Note that this doesn't make your symbol information any more complete than it already was.

Please look over your project configurations carefully and ensure:

• your .dproj files are included by sonar.sources
• the following properties are set correctly:
  • sonar.delphi.toolchain
  • sonar.delphi.compilerVersion
  • sonar.delphi.installationPath
• all relevant source code is present in the scan environment and at the correct path
  • including third party source code
  • the analyzer does not support scanning compiled DCUs

If things still don't seem quite right and you're continuing to see false positives, I would be happy to look over any new minimal reproducible cases in a new issue.

This issue will be closed once #377 is merged.

[simonegirlanda]

Ciao @Cirras, I was finally able to create a very simple project that produce the issues we are facing in our main project.

Please find enclosed:

• source code (in folder c:\work\test)
• command used to build the project: compile.bat
• command used to scan source: sonar-scanner.bat & sonar-project.properties
• log file of the analysis: scanner.log

False positive are on lines of file AgentGuardian.pas: 6: uses of Winapi.windows 13: const declaration 32: variable

These issues are all related to the line 37 where the procedure PostMessage is used and caused because this unit wasn't found:

14:57:08.844 DEBUG X cxSchedulerStorage Failed to locate unit

in the test source directory the cxSchedulerStorage.pas is renamed in cxSchedulerStorage.xpas, so the scanner ignore it.

If the cxSchedulerStorage.pas file is included in the scan analysis all the issues disappear.

This is a very common situation in our project using libraries, because we cannot have all units available as source code during build / analysis and because we want to exclude 3rd parties libraries source code from analysis.

Can the #377 change improve the robustness of this evaluation avoiding these issues?

I hope that this project can help.

very-simple-project.zip

[cirras]

in the test source directory the cxSchedulerStorage.pas is renamed in cxSchedulerStorage.xpas, so the scanner ignore it.

If the cxSchedulerStorage.pas file is included in the scan analysis all the issues disappear.

This is a very common situation in our project using libraries...

This is a weird (and wrong) thing to be doing. A .pas file is source code and a .xpas file is not. The compiler doesn't recognize it as source code and neither will we.

because we cannot have all units available as source code during build

I assume that you're compiling from DCUs and don't want to use the source code at compile-time.

The correct thing to do:

• Put DCUs on the search path.
• Put source code on the Debug source path. This makes source code available to both the debugger and SonarDelphi.

and because we want to exclude 3rd parties libraries source code from analysis

Use sonar.exclusions for this purpose. (more: Setting the initial analysis scope of your project)

Can the https://github.com/integrated-application-development/sonar-delphi/pull/377 change improve the robustness of this evaluation avoiding these issues?

SonarDelphi will only ever look at source files (extension .pas/.dpk/.dpr). That's not changing.

I hope that this project can help.

It does help in my understanding - thanks. Based on this, it's clear that your analysis issues are essentially just misconfiguration.

There's nothing for us to do, so I'm closing this issue.

[fourls]

@simonegirlanda, just to clarify - #377 will likely resolve most of your false positive issues. That being said, there may still be unexpected gaps in the analysis (false positives, false negatives, etc.) because SonarDelphi is working with very incomplete information.

For more on how to configure SonarDelphi, including preventing certain files from having issues added, the SonarDelphi manual, particularly the "Configuring Sonar projects" section, is a useful resource.

[simonegirlanda]

@fourls thank you for the clarification.

@Cirras I know that the .xpas file will not be analyzed, that was precisely the goal of my example: to reproduce what happens in our project, when not all .pas files are available. In my opinion, the weird thing is that the scanner does not complete the analysis of line 37 because it found an unrecognized symbol (AEvent.ID) in it. This is what causes false positives.

The meaning of my question about the "robustness": with change #377, will the scanner be able to do that? Of course, I understand that .DCU files are not analyzed, and I cannot expect them to be.

I sent you our current configuration file, we will read more carefully the documentation in order to understand better how tu use them.

Thank you all for your effort.

[cirras]

Hi @simonegirlanda,

@Cirras I know that the .xpas file will not be analyzed, that was precisely the goal of my example: to reproduce what happens in our project, when not all .pas files are available. In my opinion, the weird thing is that the scanner does not complete the analysis of line 37 because it found an unrecognized symbol (AEvent.ID) in it. This is what causes false positives.

The meaning of my question about the "robustness": with change #377, will the scanner be able to do that? Of course, I understand that .DCU files are not analyzed, and I cannot expect them to be.

Thanks for the clarification - I'm understanding your previous message a bit better now.

Background

SonarDelphi has always had a strong expectation that all of your source code (including library dependencies) is available in the scan environment and made visible to the analyzer. If any units are missing, the analyzer cannot form a complete semantic model of your codebase.

What's the current behavior?

As of SonarDelphi v1.15.0, name resolution of a primary expression will short-circuit as soon as a name reference cannot be resolved/disambiguated.

Example

// Bar could not be resolved, so we don't attempt to resolve Baz, Flarp, or FlimFlam.
Foo.Bar(Baz).Flarp[FlimFlam];

You can see how this would produce false positives in UnusedLocalVariable and UnusedConstant, because that may be the only usage of Baz.

The idea of doing a best-effort partial resolution has come up in the past, and I've knocked it back repeatedly because I was afraid it would mask misconfigured projects and make it harder to realize there's a problem.

What's changing in #377?

I've relaxed my position on best-effort partial resolution. Name resolution of primary expressions will no longer short-circuit, and unambiguous name references in argument lists and array accessors will still be resolved.

These changes will be released with SonarDelphi v1.16.0.

Example

// Bar could not be resolved, so we don't attempt to resolve Flarp.
// However, we still manage to resolve Baz and FlimFlam.
Foo.Bar(Baz).Flarp[FlimFlam];

Limitations

Name reference short-circuiting

There is no "best-effort" pathway for top-level name references following the unresolved name. These will simply remain unresolved.

Ambiguous Declarations

Scenario:

• Foo.Bar(Baz).Flarp[FlimFlam];
• Bar is a reference to a routine that accepts TProc<string>
• Baz is declared as follows:

  procedure Baz(Arg: string); overload;
  procedure Baz(Arg: Integer); overload;
  procedure Baz(Arg: Boolean); overload;
  

If the analyzer cannot resolve Bar, then it doesn't know that it's supposed to resolve to the first Baz overload. This name reference cannot be disambiguated, so Baz will be unresolved.

Summary

The UnusedConstant issue you've shown here (and others like it) should no longer appear.

The big caveat is that, while #377 will improve the situation in some rules (like UnusedLocalVariable and UnusedConstant) for projects with missing units or similar misconfigurations, other rules will still suffer from false positives/negatives when the semantic model is incomplete in this way.