StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
68 stars 27 forks source link

Rule0032ClearCodeunitSingleInstance' threw an exception of type 'System.InvalidCastException' #656

Closed jwikman closed 1 week ago

jwikman commented 2 weeks ago

I got below error lately:

Analyzer 'BusinessCentral.LinterCop.Design.Rule0032ClearCodeunitSingleInstance' threw an exception of type 'System.InvalidCastException' with message 'System.InvalidCastException: Unable to cast object of type 'Microsoft.Dynamics.Nav.CodeAnalysis.BoundGlobal' to type 'Microsoft.Dynamics.Nav.CodeAnalysis.IConversionExpression'.
   at BusinessCentral.LinterCop.Design.Rule0032ClearCodeunitSingleInstance.ClearCodeunit(OperationAnalysisContext ctx)
   at Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__DisplayClass54_1.<ExecuteOperationAction>b__1() in X:\Prod\Microsoft.Dynamics.Nav.CodeAnalysis\DiagnosticAnalyzer\AnalyzerExecutor.cs:line 773
   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'

The same error also occurs with BoundParameter instead of BoundGlobal in the error message.

I'm running LinterCop 0.30.25.0 according to the file version, but I just downloaded latest version... It seems as if the assets for v0.30.27.0 got wrong file version, they seem to say v0.30.25.0... So I assume that I am running v0.30.27.0 AL Language: 13.0.1027618

It's in a 500+ object solution, so it is hard to isolate to a repro, but I will dig further and try to repro...

I do not think it is a duplicate of #523, but not sure...

jwikman commented 2 weeks ago

I went back in history and found the commit where this warning seems to appear. It might have appeared after I refactored to use SecretText in some SingleInstance objects. Both as Globals and Parameters.

I tried to create a small repro, but I didn't manage to - so it must be some other parameters involved as well.

If it's not too much work, LC0000 might be a way forward?

Arthurvdv commented 1 week ago

I've added an LC0000 in the pre-release version of the LinterCop, could you check if this helps to identify where the problem occurs?

jwikman commented 1 week ago

Great @Arthurvdv!

Are there some way to have a generic catching of all unhandled exceptions?

It is always SO easy to repro after you've added LC0000!

The exception is thrown when clearing a SecretText variable:

codeunit 50101 Repro
{
    trigger OnRun()
    var
        MySecret: SecretText;
    begin
        Clear(MySecret); // <---- LC0000
    end;
}
Arthurvdv commented 1 week ago

This should now been resolved in the updated (v0.30.27) pre-release version of the LinterCop.

Are there some way to have a generic catching of all unhandled exceptions?

Unfortunately I didn't find a way to implement a generic error without slapping a Try/Catch around every method/rule.

This doesn't mean that we should keep running into exceptions. With the use of the is keyword we can casting an variable and check on the result, see the example below. This way we can handle an unexpected variable, like the SecretText in your example.

// This can cause a 'System.InvalidCastException' when the value isn't of type IConversionExpression
IConversionExpression boundConversion = (IConversionExpression)operation.Arguments[0].Value

// Safe way of casting without causing a 'System.InvalidCastException'
if (operation.Arguments[0].Value is not IConversionExpression boundConversion)
      return;
jwikman commented 1 week ago

Thanks, This indeed seems to be fixed now.

I see the challenge with the Try/Catch. And the challenge with ALWAYS checking type with is before type casting. 😅