Merge pull request #622 from Acumatica/feature/ATR-777-dev-graphs-and-graph-extensions-not-sealed

Feature/atr 777 dev graphs and graph extensions not sealed

Author: SENya1990

docs/Summary.md

@@ -113,3 +113,4 @@ Acuminator does not perform static analysis of projects whose names contain `Tes
 | [PX1110](diagnostics/PX1110.md) | The DAC has a DAC field with the `PXDBLocalizableString` attribute. Therefore, this DAC must declare a `NoteID` DAC field. | Error | Available |
 | [PX1111](diagnostics/PX1111.md) | The primary DAC of a processing view must contain the `NoteID` field. | Error | Unavailable |
 | [PX1112](diagnostics/PX1112.md) | Graphs and graph extensions with generic type parameters must be abstract. | Error | Available |
+| [PX1113](diagnostics/PX1113.md) | Graphs and graph extensions should not be `sealed` types. | Warning | Available |

docs/diagnostics/PX1113.md

@@ -0,0 +1,62 @@
+# PX1113
+This document describes the PX1113 diagnostic.
+
+## Summary
+
+| Code   | Short Description                                         |  Type   | Code Fix  |
+| ------ | --------------------------------------------------------- | ------- | --------- |
+| PX1113 | Graphs and graph extensions should not be `sealed` types. | Warning | Available |
+
+## Diagnostic Description
+
+The PX1113 diagnostic detects graphs and graph extensions marked with the `sealed` modifier.
+
+Graphs and graph extensions in Acumatica Framework must not be `sealed` types because the framework's customization mechanism depends on the ability to inherit from and extend existing business logic components. 
+When the `PXGraph.CreateInstance` factory method creates a graph instance, the Acumatica runtime dynamically generates derived types from the instantiated graph and its associated graph extensions. 
+Within these dynamically generated types, the runtime applies and combines logic from applicable graph extensions. 
+
+Declaring graphs or graph extensions as `sealed` prevents this dynamic type generation, making customization impossible and causing runtime errors when attempting to extend such types.
+This goes against Acumatica Framework design best practices which promote extensibility and reusability of business logic. Thus, the diagnostic PX1113 issues a warning for any graph or graph extension declared as `sealed`.
+
+In a rare scenario when the `sealed` modifier is intentionally used to prevent the extension of the business logic, developers should suppress the alert from PX1113 diagnostic with Acuminator suppression comment.
+
+## Code Fix Behavior
+
+The PX1113 code fix automatically removes the `sealed` modifier from the graph or graph extension declaration.
+
+## Example of Incorrect Code
+
+```C#
+using PX.Data;
+
+namespace Examples
+{
+	// Sealed graph - reports PX1113
+	public sealed class OrderGraph<TOrderDac> : PXGraph<GenericOrderGraph<TOrderDac>>
+	where TOrderDac : PXBqlTable, IBqlTable, new()
+	{
+		// Business logic
+	}
+}
+```
+
+## Example of Correct Code after Code Fix
+
+```C#
+using PX.Data;
+
+namespace Examples
+{
+	public class OrderGraph<TOrderDac> : PXGraph<GenericOrderGraph<TOrderDac>>
+	where TOrderDac : PXBqlTable, IBqlTable, new()
+	{
+		// Business logic
+	}
+}
+```
+
+## Related Articles
+
+ - [Business Logic Implementation](https://help.acumatica.com/Help?ScreenId=ShowWiki&pageid=b02bd27f-3e86-4210-9e0f-c0df7aa701d9)
+ - [Use of Generic Graph Extensions by the System](https://help.acumatica.com/Help?ScreenId=ShowWiki&pageid=eece8358-f6ea-47e8-899a-d466eccbc14e)
+ - [Reusing Business Logic](https://help.acumatica.com/Help?ScreenId=ShowWiki&pageid=76b8c160-89bd-4501-9f9f-1dabc648d417)

src/Acuminator/Acuminator.Analyzers/DiagnosticsShortName.Designer.cs

@@ -465,4 +465,7 @@
   <data name="PX1112" xml:space="preserve">
     <value>GenericNonAbstractGraphOrGraphExtension</value>
   </data>
+  <data name="PX1113" xml:space="preserve">
+    <value>SealedGraphOrGraphExtension</value>
+  </data>
 </root>

src/Acuminator/Acuminator.Analyzers/DiagnosticsShortName.resx

@@ -875,6 +875,12 @@ Reason: {2}</value>
   <data name="PX1112Title" xml:space="preserve">
     <value>Graphs and graph extensions with generic type parameters must be abstract</value>
   </data>
+  <data name="PX1113Fix" xml:space="preserve">
+    <value>Remove "sealed" modifier from the type</value>
+  </data>
+  <data name="PX1113Title" xml:space="preserve">
+    <value>Graphs and graph extensions should not be sealed types</value>
+  </data>
   <data name="SuppressDiagnosticGroupCodeActionTitle" xml:space="preserve">
     <value>Suppress the {0} diagnostic with Acuminator</value>
   </data>

src/Acuminator/Acuminator.Analyzers/Resources.Designer.cs

@@ -1,8 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
-using System.Threading;
 
 using Acuminator.Analyzers.StaticAnalysis.PXGraph;
 using Acuminator.Utilities.Common;
@@ -22,31 +22,52 @@ public class GraphAndGraphExtensionDeclarationAnalyzer : PXGraphAggregatedAnalyz
 		public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } =
 			ImmutableArray.Create
 			(
-				Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract
+				Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract,
+				Descriptors.PX1113_SealedGraphsAndGraphExtensions
 			);
 
 		public override void Analyze(SymbolAnalysisContext context, PXContext pxContext, PXGraphEventSemanticModel graphOrGraphExt)
 		{
 			context.CancellationToken.ThrowIfCancellationRequested();
-
 			CheckIfGraphOrGraphExtensionIsGenericNonAbstract(context, pxContext, graphOrGraphExt);
+
+			context.CancellationToken.ThrowIfCancellationRequested();
+			CheckIfGraphOrGraphExtensionIsSealed(context, pxContext, graphOrGraphExt);
 		}
 
 		protected virtual void CheckIfGraphOrGraphExtensionIsGenericNonAbstract(SymbolAnalysisContext context, PXContext pxContext,
 																				PXGraphEventSemanticModel graphOrGraphExt)
 		{
 			if (graphOrGraphExt.Symbol.IsAbstract || !graphOrGraphExt.Symbol.IsGenericType ||
-				graphOrGraphExt.Symbol.TypeParameters.IsDefaultOrEmpty || graphOrGraphExt.Node == null)
+				graphOrGraphExt.Symbol.TypeParameters.IsDefaultOrEmpty)
 			{
 				return;
 			}
 
-			var location = graphOrGraphExt.Node.Identifier.GetLocation().NullIfLocationKindIsNone() ??
+			var location = graphOrGraphExt.Node!.Identifier.GetLocation().NullIfLocationKindIsNone() ??
 						   graphOrGraphExt.Node.GetLocation();
 			var diagnostic = Diagnostic.Create(Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract, location,
 												graphOrGraphExt.Symbol.Name);
 
 			context.ReportDiagnosticWithSuppressionCheck(diagnostic, pxContext.CodeAnalysisSettings);
 		}
+
+		protected virtual void CheckIfGraphOrGraphExtensionIsSealed(SymbolAnalysisContext context, PXContext pxContext,
+																	PXGraphEventSemanticModel graphOrGraphExt)
+		{
+			if (!graphOrGraphExt.Symbol.IsSealed)
+				return;
+
+			var sealedToken = graphOrGraphExt.Node!.Modifiers.FirstOrDefault(m => m.IsKind(SyntaxKind.SealedKeyword));
+			var location = sealedToken != default
+				? sealedToken.GetLocation().NullIfLocationKindIsNone()
+				: null;
+
+			location ??= graphOrGraphExt.Node.Identifier.GetLocation().NullIfLocationKindIsNone() ??
+						 graphOrGraphExt.Node.GetLocation();
+			var diagnostic = Diagnostic.Create(Descriptors.PX1113_SealedGraphsAndGraphExtensions, location, graphOrGraphExt.Symbol.Name);
+
+			context.ReportDiagnosticWithSuppressionCheck(diagnostic, pxContext.CodeAnalysisSettings);
+		}
 	}
 }

src/Acuminator/Acuminator.Analyzers/Resources.resx

@@ -28,36 +28,38 @@ public class TypeModifiersFix : PXCodeFixProvider
 			new HashSet<string>
 			{
 				Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract.Id,
+				Descriptors.PX1113_SealedGraphsAndGraphExtensions.Id
 			}
 			.ToImmutableArray();
 
 		protected override Task RegisterCodeFixesForDiagnosticAsync(CodeFixContext context, Diagnostic diagnostic)
 		{
-			bool removeSealedModifier, addAbstractModifier;
+			bool addAbstractModifier;
 			string codeActionName;
 
 			if (diagnostic.Id == Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract.Id)
 			{
-				addAbstractModifier  = true;
-				removeSealedModifier = true;
-				codeActionName 		 = nameof(Resources.PX1112Fix).GetLocalized().ToString();
+				addAbstractModifier = true;
+				codeActionName 		= nameof(Resources.PX1112Fix).GetLocalized().ToString();
 			}
-			else
+			else if (diagnostic.Id == Descriptors.PX1113_SealedGraphsAndGraphExtensions.Id)
 			{
-				return Task.CompletedTask;
+				addAbstractModifier = false;
+				codeActionName 		= nameof(Resources.PX1113Fix).GetLocalized().ToString();
 			}
+			else
+				return Task.CompletedTask;
 
 			context.CancellationToken.ThrowIfCancellationRequested();
 
 			var codeAction = CodeAction.Create(codeActionName,
-											   cToken => UpdateTypeModifiers(context.Document, context.Span, removeSealedModifier, 
-																			 addAbstractModifier, cToken),
+											   cToken => UpdateTypeModifiers(context.Document, context.Span, addAbstractModifier, cToken),
 											   equivalenceKey: codeActionName);
 			context.RegisterCodeFix(codeAction, diagnostic);
 			return Task.CompletedTask;
 		}
 
-		private async Task<Solution> UpdateTypeModifiers(Document document, TextSpan span, bool removeSealedModifier, bool addAbstractModifier,
+		private async Task<Solution> UpdateTypeModifiers(Document document, TextSpan span, bool addAbstractModifier,
 														 CancellationToken cancellationToken)
 		{
 			Solution originalSolution = document.Project.Solution;
@@ -84,22 +86,22 @@ private async Task<Solution> UpdateTypeModifiers(Document document, TextSpan spa
 			if (typeDeclarations.Length <= 1)
 			{
 				var changedSolutionForNonPartialType = UpdateTypeNodeModifiersForNonPartialType(document, root, graphOrDacOrExtensionToMakePublicNode,
-																								removeSealedModifier, addAbstractModifier);
+																								addAbstractModifier);
 				return changedSolutionForNonPartialType;
 			}
 			else
 			{
 				var changedSolutionForPartialType = await UpdateTypeNodeModifiersForPartialType(document.Project.Solution, typeDeclarations,
-																						removeSealedModifier, addAbstractModifier, cancellationToken)
+																								addAbstractModifier, cancellationToken)
 														.ConfigureAwait(false);
 				return changedSolutionForPartialType;
 			}	
 		}
 
 		private Solution UpdateTypeNodeModifiersForNonPartialType(Document document, SyntaxNode root, ClassDeclarationSyntax graphOrGraphExtTypeNode, 
-																  bool removeSealedModifier, bool addAbstractModifier)
+																  bool addAbstractModifier)
 		{
-			var modifiedGraphOrGraphExtTypeNode = UpdateTypeNodeModifiers(graphOrGraphExtTypeNode, removeSealedModifier, addAbstractModifier);
+			var modifiedGraphOrGraphExtTypeNode = UpdateTypeNodeModifiers(graphOrGraphExtTypeNode, addAbstractModifier);
 
 			if (ReferenceEquals(modifiedGraphOrGraphExtTypeNode, graphOrGraphExtTypeNode))
 				return document.Project.Solution;
@@ -111,8 +113,7 @@ private Solution UpdateTypeNodeModifiersForNonPartialType(Document document, Syn
 		}
 
 		private async Task<Solution> UpdateTypeNodeModifiersForPartialType(Solution originalSolution, ImmutableArray<SyntaxReference> graphOrGraphExtDeclarations,
-																		   bool removeSealedModifier, bool addAbstractModifier, 
-																		   CancellationToken cancellation)
+																		   bool addAbstractModifier, CancellationToken cancellation)
 		{
 			var solutionEditor = new SolutionEditor(originalSolution);
 			var documentEditors = await GetAllDocumentEditorsAsync(solutionEditor, graphOrGraphExtDeclarations, cancellation).ConfigureAwait(false);
@@ -129,7 +130,7 @@ private async Task<Solution> UpdateTypeNodeModifiersForPartialType(Solution orig
 
 				foreach (var graphOrGraphExtTypeNode in graphOrGraphExtTypeNodesInDocument)
 				{
-					var modifiedGraphOrGraphExtTypeNode = UpdateTypeNodeModifiers(graphOrGraphExtTypeNode, removeSealedModifier, addAbstractModifier);
+					var modifiedGraphOrGraphExtTypeNode = UpdateTypeNodeModifiers(graphOrGraphExtTypeNode, addAbstractModifier);
 
 					if (ReferenceEquals(modifiedGraphOrGraphExtTypeNode, graphOrGraphExtTypeNode))
 						continue;
@@ -178,14 +179,13 @@ select typeDecl.GetSyntax(cancellation))
 				   .OfType<ClassDeclarationSyntax>();
 		}
 
-		private ClassDeclarationSyntax UpdateTypeNodeModifiers(ClassDeclarationSyntax graphOrGraphExtensionNode,
-																bool removeSealedModifier, bool addAbstractModifier)
+		private ClassDeclarationSyntax UpdateTypeNodeModifiers(ClassDeclarationSyntax graphOrGraphExtensionNode, bool addAbstractModifier)
 		{
 			var oldModifiers = graphOrGraphExtensionNode.Modifiers;
 			SyntaxTokenList newModifiers = oldModifiers;
 			bool modifiedAny = false;
 
-			if (removeSealedModifier && oldModifiers.Count > 0)
+			if (oldModifiers.Count > 0)
 			{
 				int sealedIndex = newModifiers.IndexOf(SyntaxKind.SealedKeyword);
 				if (sealedIndex >= 0 && sealedIndex < newModifiers.Count)

src/Acuminator/Acuminator.Analyzers/StaticAnalysis/DeclarationAnalysisGraphAndDac/GraphAndGraphExtensionDeclarationAnalyzer.cs

@@ -541,5 +541,8 @@ private static DiagnosticDescriptor Rule(string id, LocalizableString title, Cat
 
 		public static DiagnosticDescriptor PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract { get; } =
 			Rule("PX1112", nameof(Resources.PX1112Title).GetLocalized(), Category.Acuminator, DiagnosticSeverity.Error, DiagnosticsShortName.PX1112);
+
+		public static DiagnosticDescriptor PX1113_SealedGraphsAndGraphExtensions { get; } =
+			Rule("PX1113", nameof(Resources.PX1113Title).GetLocalized(), Category.Acuminator, DiagnosticSeverity.Warning, DiagnosticsShortName.PX1113);
 	}
 }

src/Acuminator/Acuminator.Analyzers/StaticAnalysis/DeclarationAnalysisGraphAndDac/TypeModifiersFix.cs

@@ -15,6 +15,8 @@
 using Microsoft.CodeAnalysis.Diagnostics;
 
 using Xunit;
+using Acuminator.Utilities.Roslyn.Semantic;
+using Acuminator.Utilities.Roslyn.Semantic.PXGraph;
 
 namespace Acuminator.Tests.Tests.StaticAnalysis.DeclarationAnalysisGraphAndDac
 {
@@ -91,6 +93,10 @@ private sealed class GraphAndGraphExtensionDeclarationAnalyzerForPX1112Tests : G
 				(
 					Descriptors.PX1112_GenericGraphsAndGraphExtensionsMustBeAbstract
 				);
+
+			protected override void CheckIfGraphOrGraphExtensionIsSealed(SymbolAnalysisContext context, PXContext pxContext, 
+																		 PXGraphEventSemanticModel graphOrGraphExt)
+			{ }
 		}
 	}
 }