Al: False detection of AS0022 "The external scope in 'MyProcedure2' cannot be removed or changed to 'internal'"

[jwikman]

Describe the bug
When we change a procedure to be an internal procedure, inside a page that has been marked as Obsolete=Pending, we get the AS0022 error.

To Reproduce
Steps and to reproduce the behavior:

1. Create a page with a couple of procedures
2. Set ObsoleteState = Pending in the page
3. Compile to get an app file
4. Copy the app file to the .alpackages folder
5. Configure appsourcecop.json for that version
6. Increase version in app.json
7. Change one procedure to internal and compile

Now AS0022 will appear.

v1.0.0.0:

page 50100 "asdf Test"
{
    ObsoleteState = Pending;
    ObsoleteReason = 'Will be removed (or modified)..';
    ObsoleteTag = '15.0';

    PageType = Card;
    SourceTable = Customer;
    Caption = 'asdf Test';
    UsageCategory = None;

    layout
    {
        area(content)
        {
            group(General)
            {
                field("No."; "No.")
                {
                    ApplicationArea = All;
                }
                field(Name; Name)
                {
                    ApplicationArea = All;
                }
            }
        }
    }

    procedure MyProcedure()
    begin
        Message('repro');
    end;

    procedure MyProcedure2()
    begin
        Message('now');
    end;

}

v1.1.0.0:

page 50100 "asdf Test"
{
    ObsoleteState = Pending;
    ObsoleteReason = 'Will be removed (or modified)..';
    ObsoleteTag = '15.0';

    PageType = Card;
    SourceTable = Customer;
    Caption = 'asdf Test';
    UsageCategory = None;

    layout
    {
        area(content)
        {
            group(General)
            {
                field("No."; "No.")
                {
                    ApplicationArea = All;
                }
                field(Name; Name)
                {
                    ApplicationArea = All;
                }
            }
        }
    }

    procedure MyProcedure()
    begin
        Message('repro');
    end;

    internal procedure MyProcedure2()
    begin
        Message('now');
    end;

}

Expected behavior
Since the object was obsolete pending, no issues should appear...
We should be able to change the scope of a procedure if the object was obsolete pending.

Screenshots
N/A

5. Versions:

• AL Language: 5.0.270354
  
• Business Central: N/A

[jwikman]

This issue still exists in 5.0.271898

[jwikman]

A workaround is to obsolete only the procedure, and then change the scope in next release... But I believe that both scenarios is valid. The change in the public API is the same.

[qutreson]

Thank you for your feedback on the rules! We have discussed internally this scenario and for now we have decided to not allow it in the AppSourceCop. This is a valid scenario, but if the AppSourceCop stopped validating for breaking changes in obsolete objects, we would fear to loosen too much the validation. The main reason for that is that obsolete objects are still part of the public interface as some dependent extensions might still rely one them, so the AppSourceCop should then keep on validating their members for breaking changes. Marking an object as obsolete skips some of the static validation (like validating affixes with AS0011) in order to allow fixing these diagnostics, but should not skip the backward compatibility validation. In the future, when we collect more feedback and experience running with the AppSourceCop, we might come back on this decision.

The other reason for now allowing it for now is that two alternative flows are supported to achieve a similar result:
- Obsoleting the object procedure itself:
1. Version 1 introduces a new codeunit and a method
2. Version 2 marks the procedure as obsolete
3. Version 3 makes a breaking change on the procedure and removes the obsolete attribute on the procedure.
- Obsoleting the object itself:
1. Version 1 introduces a new codeunit and a method
2. Version 2 marks the codeunit as obsolete
3. Version 3 makes a breaking change on the procedure and removes the obsolete property on the codeunit.

I will mark this issue as a suggestion for future improvement.

[jwikman]

Fair enough. I understand that there are a lot of complexity in this.

With all changes released this morning we have at least a way forward, even if some changes has to be done in more steps than we first thought of.

But the following scenario (from your comment "The following scenarios should not raise any diagnostics" in #5868) results in AS0018, but I really think it should be valid :

Removing an obsolete object/procedure

• Version 1: codeunit is obsolete
• Version 2: codeunit is removed

[qutreson]

Let me double check that scenario. It should not raise any error, if it does, it looks like a bug

[qutreson]

I can reproduce the issue. We will work on a fix for it

[qutreson]

@jwikman I am closing this issue as it has been fixed in the latest version of the AL extension in the marketplace.