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

CheckClearPostCodeCityCounty - Possible overflow assigning #4838

Closed fvet closed 4 years ago

fvet commented 5 years ago

What is the standard policy of BC on AA0139 errors?

When procedures have parameters containing no fixed text length, the possible overflow warning must normally be dealt with after the procedure has been called and the return values are saved into the table fields. However, since the parameters in the CheckClearPostCodeCityCounty procedure actually reference the actual table fields, there's no possibility to avoid the AA0139 overflow warning.

Example : Adding the following code on 'postcode/city/country' fields, results in lots of AA0139 warnings.

            trigger OnValidate()
            var
                Postcode: record "Post Code";
            begin
                PostCode.CheckClearPostCodeCityCounty("City", "Post Code", "County", "Country/Region Code", xRec."Country/Region Code");
            end;

{ "code": "AA0139", "message": "Possible overflow assigning 'Text' to 'Text[30]'.", }

Trying to get the most out of CodeCop and thus not ignoring the AA0139 rule, how can this warning be fixed (as well as in standard BC) without the use of extra variables?

gregoral commented 4 years ago

There are other functions with the same issue. Is it possible to get a short description of how this is going to be addressed?

If the fix will affect this specific function and not others, should I report the same issue with other functions?

Another example is: MailManagement.ValidateEmailAddressField("E-Mail");

Consider a function that ( currently ) returns the same or shorter string than the original. But that behaviour is not guaranteed and might change with a future update. How are we supposed to handle that?

Example:

var EMail: Text;
begin
    EMail := "E-Mail";
    MailManagement.ValidateEmailAddressField(EMail);
    "E-Mail" := CopyStr(EMail, 1, MaxStrLen("E-Mail"));
end;

If we handle it like this, we risk problems in the future. Perhaps not with MailManagement.ValidateEmailAddressField function but in general.

Another example:

Instead of this:

trigger OnLookup()
begin
    PostCode.LookupPostCode(City, "Post Code", County, "Country/Region Code");
end;

We have to write this:

trigger OnLookup()
var
    CityBuffer: Text;
    CountyBuffer: Text;
begin
    CityBuffer := City;
    CountyBuffer := County;

    PostCode.LookupPostCode(CityBuffer, "Post Code", CountyBuffer, "Country/Region Code");

    if (StrLen(CityBuffer) > MaxStrLen(City)) then
        Error(CityOverflowErr, City, FieldCaption(City));

    if (StrLen(CountyBuffer) > MaxStrLen(County)) then
        Error(CountyOverflowErr, County, FieldCaption(County));

    City := CopyStr(CityBuffer, 1, MaxStrLen(City));
    County := CopyStr(CountyBuffer, 1, MaxStrLen(County));
end;

Ideally:

Example:

[TryFunction][OverflowSafe]
procedure ValidateEmailAddressField(var EmailAddress: Text)
var
   DefaultEmailLbl: Label 'someone.or.something@somwhere.com, MaxLength = 200';
begin
   if(StrLen(DefaultEmailLbl) > MaxStrLen(EmailAddress)) then Error('...');
   EmailAddress := DefaultEmailLbl;
   //...
end;

NOTE: MaxStrLen(EmailAddress) in the example above should return the max length of the original variable passed in as a parameter and not 2147483647.

CodeCop could recognize this and suppress the warning.

MarcHansenMicrosoft commented 4 years ago

As the issue will be a breaking change, it was decided not to be fixed.