microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
744 stars 245 forks source link

Question : AA0073 The name of temporary variable must be prefixed with Temp (procedure arguments) #6227

Closed fvet closed 2 years ago

fvet commented 4 years ago

Title The AA0073 warning is raised for local / global variables. Question is why the warning is not applied to function parameters too?

Code sample

(Good) In the below procedure, AA0073 shows for DemoBaseLocalRec, since the temp prefix is missing.

procedure MyProcedure(DemoBase: record "Demo Base");
    var
        DemoBaseLocalRec: record "Demo Base" temporary;
    begin

    end;

(Question / bad?) In the below procedure, AA0073 does not show for DemoBase, although it also concerns a temporary record. Is there any particular reason not to prefix temporary function arguments with temp?

procedure MyProcedure4(DemoBase: record "Demo Base" temporary);
    begin
    end;

The same could apply for integration events. (although new events might have to be created to avoid breaking changes?)

[IntegrationEvent(false, false)]
    local procedure MyIntegrationEvent(DemoBase: record "Demo Base" temporary);
    begin
    end;
dzzzb commented 4 years ago

Is there any particular reason not to prefix temporary function arguments with temp?

I can't see one.

One quirk is that if the arg (unlike yours) is var, whether or not it's actually temporary is determined by how the passed-in record was declared. So a var argument can be temporary or not, depending on the caller, not on the callee. In your case, it's by-value, so the temporary matters - e.g. if someone passed a 'real' record, which got copied, then the function did an Insert(), it'd put it in a temp table.

Saying that, if someone puts var Foo temporary, even if the temporary has no practical meaning, to me it has semantic meaning that it should always be a temporary record that gets passed in... in which case, the warning should persist.

So, basically, no, no reason that I can see :-)

ref.: https://vjeko.com/2012/11/01/some-tips-and-hints-about-temporary-tables/

fvet commented 4 years ago

Somewhat related:

With the new TableType = Temporary, we are no longer forced to mark the record variable as temporary, although I would find it still usefull to get the AA0073 warning, if not prefixed by Temp. I think the AA0073 warning should be respected also for tables explicitly declared as temporary on the object itself.

dzzzb commented 4 years ago

That is a very good point too đź‘Ť

mjmatthiesen commented 4 years ago

Just disable AA0073. It's a dumb rule.

rdebath commented 3 years ago

I see an issue with a Tabletype = Temp triggering this message. Specifically, that sort of table will traditionally be named XMLBuffer or similar. This makes the name TempXMLBuffer. While the Temp affix is a good (true) Hungarian notation flag because you really don't want to mix a temp table and a perm table that name seems just a bit repetitive.

dzzzb commented 3 years ago

That's a good point: since such tables can never be 'real', arguably the warning nature of the Temp prefix is not required (also it's kinda opposite; maybe let's prefix all non-temp tables with Real ;-P )

rdebath commented 3 years ago

... "As Real" ... :grin: Weeeelll some other languages do by saying Object.SQLInsert as opposed to Object.HashInsert.

Though if a type is doing the job, you don't want decoration.

dzzzb commented 3 years ago

new issue about TableType = Temporary and AA0237: https://github.com/microsoft/AL/issues/6602

mazhelez commented 2 years ago

Thanks for reporting this issue. Sorry we haven’t completed it yet, but we’ve had to prioritize elsewhere. We’re planning to give the CodeCop engine and its rules an overhaul in a future major release. Thanks for your patience.

Going forward we will not track progress on CodeCop issues here on GitHub. Hence, we are closing this issue.