LC0053 - Issue with internal procedures for testability

[CHovenbitzer]

Hey, I have an issue with the rule LC0053 "The internal method is only used in the object in which it is declared. ". I frequently use the internal access modifier to allow my test function to direcly call them. This allows me to write tests more granular but the rule does not like that at all.

I'm not sure how to fix this.

[rvanbekkum]

Hi, it was a deliberate choice for the moment to always have this diagnostic raised if the internal procedure is not used in the current project.
The suggested way of fixing it is by suppressing individual occurrences of the diagnostic using #pragma warning disable LC0052, LC0053 for procedures that you really need for your unit tests.

https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0052#internalsvisibleto

If you have other suggestions, feel free to share them though.
I don't think there is another way around it though, because the main app can also be build completely separately from the unit tests.
You could choose to completely disable the rule, but then you don't have the benefits it offers either.

[CHovenbitzer]

References of procedures are shared across both the normal and test app. Perhaps the linter is able to differentiate between the two apps when checking references? That way at least in VSCode the info wouldn't be raised but we would have a different behaviour when compiling the app. Just sharing my thoughts here.

The way it is currently the rule raises more "false positives" than anything else so I will probably disable the rule entirely.

[rvanbekkum]

Yes, that's right. I am not sure if we will be able to have the rule check across multiple projects as I suspect the compiler compiles per project and does not bother with checking your workspace setup. But it could be something to check.
For our projects we are not really bothered by this, but I can imagine others might more heavily rely on internal procedures in their unit tests.

I think a first good step could be to let the default behaviour of the rule be "if internalsVisibleTo is not empty, then disable the rule", but add an ignoreInternalsVisibleTo flag in LinterCop.json so that you can enable rules LC0052 and LC0053 regardless, if you want to.

Second step would be investigating if we could realize what you suggest.

What do you think?

[CHovenbitzer]

Sounds great. That would be enough for me already.

I am not sure about the second step though. I think it's more important to have consistent behaviour for both compiling and VSCode and when compiling the app (like in pipelines) you don't have the testapp yet so there should be no way of knowing cross app references. Sounds like a lot of effort for minimal effect :D
Checking the app.json for internalsVisibleTo should be enough in my opinion.

I'll look into the code later, maybe I can find a solution to this myself and create a PR.

[Arthurvdv]

I understand the issue on having this rule raised on internal methods that are used for testing only.

To answer your question: I believe in theory it would be possible for the code analyzer to verify this in a multi-root workspace, and have this checked cross the App and the Test-App. I'm quite sure this will not work during compiling in a build pipeline and then could break pipelines out there.

if internalsVisibleTo is not empty, then disable the rule

I'm not sure if this ia a good idea. In our build for release pipelines we tent to remove the internalsVisibleTo from the app.json in the pipeline, so our shipped artifact doesn't have this property set. Again I believe this could break pipelines out there.

Checking the app.json for internalsVisibleTo should be enough in my opinion.

I would suggest to apply a custom ruleset on the workspace when setting the internalsVisibleTo property in the app.json. Personally I would follow the example as Microsoft does and add an comment (and a #pragma in this case) that this event is directly called from the test app.

    /// <summary>
    /// Raises an event to be able to change the return of IsGuiAllowed function. Used for testing.
    /// </summary>
    [InternalEvent(false)]
    procedure OnBeforeGuiAllowed(var Result: Boolean; var Handled: Boolean)
    begin
    end;

https://github.com/StefanMaron/MSDyn365BC.Code.History/blob/0e81506108e11853a3630681e63561c2268a4e70/system%20application/source/System%20Application/Confirm%20Management/src/ConfirmManagementImpl.Codeunit.al

[CHovenbitzer]

I can see your concerns with the internalsVisibleTo as this isn't a perfect indicator but I can't see how this might break pipelines The change would only make the rule more restrictive which would result in fewer infos (or warning/errors depending on the ruleset I suppose) and that should never result in a pipeline failure- on a technical level.

I can also just set this rule on ignore globally but I think this rule catches a lot of false positives just by the nature of the rule.

[StefanMaron]

@Arthurvdv why are we always talking about internals visible to for test apps? That's not even the intention of that setting.

The intention is to make the internals visible to an API app. That setting would then also need to get shipped.

I can also imagine that partners might have multi app architectures depending on that setting, also persisting it in the app.json

So I am with @CHovenbitzer, when the internals are visible they should be treated like public's

[Arthurvdv]

why are we always talking about internals visible to for test apps?

Valid point! I've looked again at the documentation of the internalsVisibleTo property of the app.json and you're right;

The InternalsVisibleTo setting will expose your internal objects to any extension with the given name, publisher, and ID. Access modifiers are not designed to be used as a security boundary, but for API development.
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/analyzers/pertenantextensioncop-pte0012

We also, just as @CHovenbitzer mentioned, are using this for our test-apps and I didn't realize that's not the intention of that setting 😳

Seems doable that when there's one (or multiple) Apps defined in the internalsVisibleTo property of the app.json to skip these diagnostics

• LC0052 - The internal method is declared but never used.
• LC0053 - The internal method is only used in the object in which it is declared. Consider making the procedure local.

I'll mark this as enhancement to look into this. @CHovenbitzer if you have the time you're more then welcome to create a PR for this, otherwise I'll try to look into this in the next week.

[ChristianHovenbitzer]

I've got some really weird issues now. When booting up a repo without internalsVisibleTo I get errors across multiple rules inlcuding LC0044, LC0052, LC0033.

Weirdly enough these issues get resolved when I set the internalsVisibleTo and don't come up again when I remove the property again.

Also I'm not sure how this is connected to the change I made. I will look into this more closely.
Update: There is a new version of the AL Language extension. That could be the issue

[{

	"resource": "/c:/git/Infox_API/app/app.json",
	"owner": "_generated_diagnostic_collection_name_#0",
	"code": "AD0001",
	"severity": 4,
	"message": "Analyzer 'BusinessCentral.LinterCop.Design.Rule0033AppManifestRuntimeBehind' threw an exception of type 'System.MissingMethodException' with message 'System.MissingMethodException: Method not found: 'Microsoft.Dynamics.Nav.CodeAnalysis.Packaging.NavAppManifest Microsoft.Dynamics.Nav.Analyzers.Common.AppSourceCopConfiguration.AppSourceCopConfigurationProvider.GetManifest(Microsoft.Dynamics.Nav.CodeAnalysis.Compilation)'.\r\n   at BusinessCentral.LinterCop.Design.Rule0033AppManifestRuntimeBehind.CheckAppManifestRuntime(CompilationAnalysisContext ctx)\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__DisplayClass49_1.<ExecuteCompilationActionsCore>b__0() in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 625\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock(DiagnosticAnalyzer analyzer, Action analyze, Nullable`1 info) in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 1088'",
	"source": "AL",
	"startLineNumber": 1,
	"startColumn": 1,
	"endLineNumber": 1,
	"endColumn": 1
}]

[{
	"resource": "/c:/git/Infox_API/app/app.json",
	"owner": "_generated_diagnostic_collection_name_#0",
	"code": "AD0001",
	"severity": 4,
	"message": "Analyzer 'BusinessCentral.LinterCop.Design.Rule0044AnalyzeTransferFields' threw an exception of type 'System.InvalidCastException' with message 'System.InvalidCastException: Unable to cast object of type 'Microsoft.Dynamics.Nav.CodeAnalysis.Syntax.CodeunitSyntax' to type 'Microsoft.Dynamics.Nav.CodeAnalysis.Syntax.TableExtensionSyntax'.\r\n   at BusinessCentral.LinterCop.Design.Rule0044AnalyzeTransferFields.AnalyzeTableExtension(SyntaxNodeAnalysisContext ctx)\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__DisplayClass53_1.<ExecuteSyntaxNodeAction>b__1() in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 752\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock(DiagnosticAnalyzer analyzer, Action analyze, Nullable`1 info) in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 1088'",
	"source": "AL",
	"startLineNumber": 1,
	"startColumn": 1,
	"endLineNumber": 1,
	"endColumn": 1
}]

[{
	"resource": "/c:/git/Infox_API/app/app.json",
	"owner": "_generated_diagnostic_collection_name_#0",
	"code": "AD0001",
	"severity": 4,
	"message": "Analyzer 'BusinessCentral.LinterCop.Design.Rule0052InternalProceduresNotReferencedAnalyzer' threw an exception of type 'System.MissingMethodException' with message 'System.MissingMethodException: Method not found: 'Microsoft.Dynamics.Nav.CodeAnalysis.Packaging.NavAppManifest Microsoft.Dynamics.Nav.Analyzers.Common.AppSourceCopConfiguration.AppSourceCopConfigurationProvider.GetManifest(Microsoft.Dynamics.Nav.CodeAnalysis.Compilation)'.\r\n   at BusinessCentral.LinterCop.Design.Rule0052InternalProceduresNotReferencedAnalyzer.MethodSymbolAnalyzer..ctor(CompilationAnalysisContext compilationAnalysisContext)\r\n   at BusinessCentral.LinterCop.Design.Rule0052InternalProceduresNotReferencedAnalyzer.CheckApplicationObjects(CompilationAnalysisContext compilationAnalysisContext)\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__DisplayClass49_1.<ExecuteCompilationActionsCore>b__0() in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 625\r\n   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock(DiagnosticAnalyzer analyzer, Action analyze, Nullable`1 info) in X:\\Prod\\Microsoft.Dynamics.Nav.CodeAnalysis\\DiagnosticAnalyzer\\AnalyzerExecutor.cs:line 1088'",
	"source": "AL",
	"startLineNumber": 1,
	"startColumn": 1,
	"endLineNumber": 1,
	"endColumn": 1
}]

[rvanbekkum]

I suspect the build pipeline has some strange issues with resolving methods from the Microsoft.Dynamics.Nav.Analyzers.Common.dll 🤔 Not sure why that would be.

'System.MissingMethodException: Method not found: 'Microsoft.Dynamics.Nav.CodeAnalysis.Packaging.NavAppManifest Microsoft.Dynamics.Nav.Analyzers.Common.AppSourceCopConfiguration.AppSourceCopConfigurationProvider.GetManifest(Microsoft.Dynamics.Nav.CodeAnalysis.Compilation)

^ I saw this in both the error messages you shared for rule 033 and 052.
A call to AppSourceCopConfigurationProvider.GetManifest should work fine, and if you build the LinterCop.dll on your own system it works fine. But when the GitHub actions build pipeline builds it, then it seems you can get this exception at runtime.
Not sure why it wouldn't be able to find the method, but it seems to be an issue with the GitHub actions build pipeline.

[Arthurvdv]

@ChristianHovenbitzer
It seems that the pre-release of the AL Language has an breaking change, that's why also the LC0033 rule now throws an exception. Probably the method is moved around, I'll see what I can do to find the replacement.

[Arthurvdv]

@ChristianHovenbitzer The function is moved to the new ManifestHelper: f55b06a

I think I now need to figure out howto add preprocessor directives, cause now the build fails on the current (v12).

[Arthurvdv]

Preprocessor directives are in place and the latest release should now work again.

[Arthurvdv]

The v0.30.13 version of the LinterCop is now released, where I believe this issue is now resolved. If you still encounter issues, feel free to reopen this issue (or create new one).