fix(fc0007,pc0038,lc0089,lc0090) Inconsistent handling of AL built-in methods that unconditionally end execution - #464
Closed
MODUSCarstenScholling wants to merge 6 commits into
Conversation
Resolve Error and FieldError calls semantically across PC0038, FC0007, and LC0089/LC0090. This preserves cognitive-complexity increments for user-defined methods with the same names, supports case-insensitive AL method names, and removes the obsolete ThrowError terminator. Add focused regression coverage and document the LC0089/LC0090 design. Fixes ALCops#463
MODUSCarstenScholling
changed the title
Arthurvdv
added a commit
to Arthurvdv/Analyzers
that referenced
this pull request
Aug 25, 2026
…ail to bind PR ALCops#464 moved guard-clause detection for Error/FieldError from the lexical guardClauseExitCommands set to FlowTerminatingBuiltIns, which requires MethodKind.BuiltInMethod. When an argument does not bind (undefined variable, wrong arity, mid-edit), Binder.CreateBadCall takes the methods.Length != 1 branch for the two-overload built-ins Dialog.Error, Table.FieldError and FieldRef.FieldError and returns an ErrorMethodSymbol with MethodKind.Method. The classifier rejected it, the guard scored +1 and LC0089/LC0090 flickered while typing. IsGuardExpression now runs in four stages: lexical exit commands, a syntactic callee-name pre-filter (only Error/FieldError reach the binder), semantic resolution, and an unbound fallback that keeps the guard unless the call positively binds to a user-defined procedure (DeclaringSyntaxReference != null) or the receiver is typed as a user application object. - Add error-tolerant fixtures proving Error(UndefinedVar) and Rec.FieldError(UndefinedField) stay guards, and Handler.Error(UndefinedVar) on a user codeunit still does not. - Fix stale comment in IsGuardInvocation. - Sync lc0089-cognitive-complexity instructions (design decisions, known issues, roadmap, test coverage). Refs ALCops#464, ALCops#463 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ail to bind PR ALCops#464 moved guard-clause detection for Error/FieldError from the lexical guardClauseExitCommands set to FlowTerminatingBuiltIns, which requires MethodKind.BuiltInMethod. When an argument does not bind (undefined variable, wrong arity, mid-edit), Binder.CreateBadCall takes the methods.Length != 1 branch for the two-overload built-ins Dialog.Error, Table.FieldError and FieldRef.FieldError and returns an ErrorMethodSymbol with MethodKind.Method. The classifier rejected it, the guard scored +1 and LC0089/LC0090 flickered while typing. IsGuardExpression now accepts an invocation target whose name is in FlowTerminatingBuiltIns.MethodNames and has no DeclaringSyntaxReference. Built-ins and the synthesized ErrorMethodSymbol both satisfy this; only user-defined procedures carry a DeclaringSyntaxReference, and since AL has no user overloads a user procedure with bad arguments still binds to its own symbol and stays demoted. - Add error-tolerant fixtures proving Error(UndefinedVar) and Rec.FieldError(UndefinedField) stay guards, and Handler.Error(UndefinedVar) on a user codeunit still does not. - Fix stale comment in IsGuardInvocation. - Sync lc0089-cognitive-complexity instructions (design decisions, known issues, roadmap, test coverage). Refs ALCops#464, ALCops#463 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…anticModel The AL SDK's Compilation.GetSemanticModel is not cached: it returns new SyntaxTreeSemanticModel(this, syntaxTree) on every call (Compilation.cs:957). Calling it once per analyzed code block therefore re-bound each procedure from scratch. CodeBlockAnalysisContext already carries the semantic model for its tree, and the analyzer uses context.SemanticModel elsewhere in the same file. - Drop the Compilation parameter from AnalyzeCognitiveComplexity. - Sync lc0089-cognitive-complexity instructions (design decision, architecture, remove the roadmap entry). Refs ALCops#464, ALCops#463 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 25, 2026
Member
|
Thank you for creating a PR for this. Instead of letting Claude write a review comment I wanted to experiment with letting it create a PR, so it created four (stacked) PR's based on the code review. Would love to hear if this works for you. |
perf(LC0089): use context.SemanticModel instead of Compilation.GetSemanticModel
…n.GetSemanticModel"
…-context-semantic-model Revert "perf(LC0089): use context.SemanticModel instead of Compilation.GetSemanticModel"
Contributor
Author
|
Thanks for trying this out, Arthur. I’m sorry, but I’m going to close the PR for now. My own review didn’t confirm all of the findings and found gaps, and I also accidentally merged the PRs in the wrong order. Unfortunately, I don’t have enough time at the moment to explore this workflow further, but I appreciate the experiment and the effort you put into it. |
Member
|
I’m sorry, this wasn’t the outcome I was expecting. I’ll reach out to you directly so we can align and make sure we’re on the same page for future PRs. |
Contributor
Author
|
Looking forward to it. But the only issue here is that directly the first PR was wrong in my opinion. Like an AI suggesting the following 10 steps where step 1 already contains errors and changes all steps after. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the inconsistent handling of AL built-in methods that unconditionally end execution.
FlowTerminatingBuiltInsas the shared semantic classifier for built-inErrorandFieldError.Record.FieldErrorandFieldRef.FieldErroras terminating return paths.FieldErrorconsistently withError.ErrororFieldError.FieldRef.FieldError.ThrowErrorentry.Validation
CognitiveComplexity: 18 passednetstandard2.1,net8.0,net10.0): 0 errorsFixes #463