Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule: out arguments should not be assigned before the call #133

Open
3 tasks done
Chadehoc opened this issue Dec 11, 2023 · 3 comments
Open
3 tasks done

New rule: out arguments should not be assigned before the call #133

Chadehoc opened this issue Dec 11, 2023 · 3 comments
Labels
feature New feature or request rule Improvements or additions to rules

Comments

@Chadehoc
Copy link

Chadehoc commented Dec 11, 2023

Prerequisites

  • This rule has not already been suggested.
  • This should be a new rule, not an improvement to an existing rule.
  • This rule would be generally useful, not specific to my code or setup.

Suggested rule title

OutArgumentShouldNotBeCalledInitialized

Rule description

The discussion at the end of #127 leads me to propose this as a separate rule; but I would prefer it as a part of the rule proposed in #132, because it also has to do with the possible confusion between var and out arguments (an integrated rule could be called OutAndVarArgumentConfusion).

The Delphi compiler does not actually make a difference between out and var. This is confusing, and can lead to accepting var as initializing the variable passed to it (rule #132 proposed against this). Another confusion is passing an initialized variable as out argument, which means that the argument should actually be var. This rule aims to detect that case.

Should be warned against:

var LVal: Integer := Default(TMyRecord); //useless
FillMyRecord({out argument} LVal); // an out argument should come out initialized

An exception is when the variable is actually used before the call. Then it is just a reuse of an existing variable.

var LVal: Integer := 12;
UseTheValue(LVal);
FillMyRecord({out argument} LVal); // this is a variable being reused, ok

Rationale

Track the confusion between out and var arguments. While there is no difference for the compiler, there is a huge difference in intent and readability.

@Chadehoc Chadehoc added feature New feature or request rule Improvements or additions to rules triage This needs to be triaged by a maintainer labels Dec 11, 2023
@Cirras
Copy link
Collaborator

Cirras commented Dec 12, 2023

Makes sense to me - thanks!

For the rule title, I'm thinking: RedundantOutArgumentInitialization

@Cirras Cirras removed the triage This needs to be triaged by a maintainer label Dec 12, 2023
@Cirras Cirras changed the title OUT arguments should not be assigned before the call New rule: out arguments should not be assigned before the call Dec 19, 2023
@sglienke
Copy link

sglienke commented Jan 22, 2024

The Delphi compiler does not actually make a difference between out and var.

This is not true - when the type of the out parameter is managed the compiler clears the variable before the call.
Also, Delphi allows reading an out parameter inside the routine before it has been assigned and it does not ensure that such a parameter is set. This means for this proposed rule to be correct there also would need to be rules that ensure that the parameter is not read and that it is being set on all code paths.
That means if the other rules are not also executed this rule could actually lead to defect if it would warn about an unnecessary initialization which is actually needed because either the value is being read or not being set in some code path.

@Chadehoc
Copy link
Author

This means for this proposed rule to be correct there also would need to be rules that ensure that the parameter is not read and that it is being set on all code paths.

I think it's what #134 is for (ensure that the parameter is not read), and that missing out results are detected by rule "Routine results should be assigned a value".

It means #134 should be finished before this new rule is created.

That means if the other rules are not also executed this rule could actually lead to defect if it would warn about an unnecessary initialization which is actually needed because either the value is being read or not being set in some code path.

Interesting point. I don't know what the policy is for a rule that should imply another one.

I would argue that if an out parameter is used as if it was var, I would prefer to be warned, rather than have it ignored by fear of an unappropriate correction.

In this particular case, the only way the unappropriate correction would not be detected by sonar-delphi would be because an essential rule has been explicitely disabled (detecting uninitialized variables or unassigned results), provided #134 is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request rule Improvements or additions to rules
Projects
None yet
Development

No branches or pull requests

3 participants