Incremental and porting updates by marknfawaz · Pull Request #110 · aws/cta

marknfawaz · 2021-04-23T23:47:50Z

Related issue

Closes: #109

jonlouie · 2021-04-26T01:37:49Z

src/CTA.Rules.Models/Tokens/NodeToken.cs

+        public List<GenericAction> AllActions
+        {
+            get
+            {
+                var allActions = new List<GenericAction>();
+                allActions.AddRange(AttributeActions);
+                allActions.AddRange(AttributeListActions);
+                allActions.AddRange(MethodDeclarationActions);
+                allActions.AddRange(ClassDeclarationActions);
+                allActions.AddRange(InterfaceDeclarationActions);
+                allActions.AddRange(ElementAccessActions);
+                allActions.AddRange(MemberAccessActions);
+                allActions.AddRange(IdentifierNameActions);
+                allActions.AddRange(InvocationExpressionActions);
+                allActions.AddRange(MemberAccessActions);
+                allActions.AddRange(UsingActions);
+                allActions.AddRange(ObjectCreationExpressionActions);
+                allActions.AddRange(NamespaceActions);
+                return allActions;
+            }
+        }


Is there a specific reason or requirement for creating a new List when this property is called? If not, convert allActions to a private class variable and reassign a value to it in the getter to prevent a potential memory leak.

If this behavior is intentional, it may be more appropriate to convert this property to a method as properties are used to retrieve state rather than generate new objects.

I'm not sure I understand. How would this cause a memory leak?

Memory leak might not be the best term here, but my thought is that it's possible to inadvertently consume more resources than expected. See the below example for what I was thinking, but if this is a nonconcern, we can forgo this modification.

Trivial example:

var allActions1 = nodeToken.AllActions; var allActions2 = nodeToken.AllActions;

With the current implementation, this would allocate 2x the memory than the suggested implementation:

private List<GenericAction> _allActions; public List<GenericAction> AllActions { get { _allActions = new List<GenericAction>(); _allActions.AddRange(AttributeActions); _allActions.AddRange(AttributeListActions); _allActions.AddRange(MethodDeclarationActions); _allActions.AddRange(ClassDeclarationActions); _allActions.AddRange(InterfaceDeclarationActions); _allActions.AddRange(ElementAccessActions); _allActions.AddRange(MemberAccessActions); _allActions.AddRange(IdentifierNameActions); _allActions.AddRange(InvocationExpressionActions); _allActions.AddRange(MemberAccessActions); _allActions.AddRange(UsingActions); _allActions.AddRange(ObjectCreationExpressionActions); _allActions.AddRange(NamespaceActions); return _allActions; } }

As discussed, this wouldn't allocate 2x memory because it's a local variable that gets disposed after the getter exits. We can look into this and the same behavior in fileactions to see if there's a better way to do it.

jonlouie · 2021-04-26T01:52:40Z

src/CTA.Rules.ProjectFile/ProjectFileCreator.cs

+        private readonly string _portingInfoItemGroupTemplate =
+@"<ItemGroup Label=""PortingInfo"">
+<!-- DO NOT REMOVE WHILE PORTING
+{0}
+-->
 </ItemGroup>";


To an end user, the messaging here is a little vague. What do you think about updating the ItemGroup Label to PortingAssistantMetadataReferences and/or updating the comment message to ADDED BY PORTING ASSISTANT. DO NOT REMOVE UNTIL PORTING IS COMPLETE. (or something similar that makes more sense)?

This would give the user context as to what these commented out references are, how they got there, and what their significance is.

Good idea. Please create an issue for this, because it'll require a change here and on codelyzer. we will prioritize this based on availability.

Sure, will create those issues

jonlouie · 2021-04-26T03:59:54Z

src/CTA.Rules.Update/CodeReplacer.cs

+                                    //If true, line endings and spaces need to be normalized:
+                                    //if (result != root.ToFullString())
+                                    //{
+                                    //    File.WriteAllText(sourceFileBuildResult.SourceFileFullPath, result);
+                                    //}


Delete unused comments/code

jonlouie · 2021-04-26T05:50:17Z

src/CTA.Rules.Update/SolutionRewriter.cs


+        public SolutionRewriter(IDEProjectResult projectResult, List<ProjectConfiguration> solutionConfiguration)


Include doc comments

jonlouie · 2021-04-26T05:50:26Z

src/CTA.Rules.Update/SolutionRewriter.cs


+        public List<IDEFileActions> RunIncremental(RootNodes projectRules, List<string> updatedFiles)


Include doc comments

jonlouie · 2021-04-26T06:08:05Z

src/CTA.Rules.Models/IDEFileActions.cs

+
+namespace CTA.Rules.Models
+{
+    public class IDEFileActions


Could you add a doc comment for the class that describes its use and how it differs from other file actions?

Also, would it make sense to rename anything with the prefix IDE to something more generic? Dependencies shouldn't be aware of the libraries or projects that consume them, but I also don't have full context here about how this model is consumed.

We don't usually add doc comments for models. We can document our models classes at a later stage if needed.

jonlouie · 2021-04-26T06:12:07Z

src/CTA.Rules.ProjectFile/ProjectFileCreator.cs

-
-            var itemGroupContent = string.Format(_itemGroupTemplate, content);
+
+            var currentGroupTemplate = itemGroupTemplate ?? _itemGroupTemplate;


nit: rename _itemGroupTemplate to _defaultItemGroupTemplate

jonlouie · 2021-04-26T06:25:37Z

src/CTA.Rules.Update/CodeReplacer.cs

+                        if (_projectConfiguration.PortCode)
                        {
-                            File.WriteAllText(sourceFileBuildResult.SourceFileFullPath, result);
-                        }
-                        var processedActions = ValidateActions(oneRewriter.allActions, result);
-                        processedActions = AddActionsWithoutExecutions(currentFileActions, oneRewriter.allActions);
-
-                        if (!actionsPerProject.TryAdd(sourceFileBuildResult.SourceFileFullPath, processedActions))
+                            ActionsRewriter oneRewriter = new ActionsRewriter(sourceFileBuildResult.SemanticModel, sourceFileBuildResult.PrePortSemanticModel, sourceFileBuildResult.SyntaxGenerator, currentFileActions.FilePath, currentFileActions.AllActions);
+                            root = oneRewriter.Visit(root);
+                            var result = root.NormalizeWhitespace().ToFullString();
+
+                            if (!_projectConfiguration.IsMockRun)
+                            {
+                                File.WriteAllText(sourceFileBuildResult.SourceFileFullPath, result);
+                            }
+
+                            var processedActions = ValidateActions(oneRewriter.allExecutedActions, result);
+                            processedActions = AddActionsWithoutExecutions(currentFileActions, oneRewriter.allExecutedActions);
+
+                            if (!actionsPerProject.TryAdd(sourceFileBuildResult.SourceFileFullPath, processedActions))
+                            {
+                                throw new FilePortingException(sourceFileBuildResult.SourceFilePath, new Exception("File already exists in collection"));
+                            }
+                        }                       
+                        else
                        {
-                            throw new FilePortingException(sourceFileBuildResult.SourceFilePath, new Exception("File already exists in collection"));
+                            if (currentFileActions != null)
+                            {
+                                currentFileActions.NodeTokens.ForEach(nodetoken =>
+                                {
+                                    ActionsRewriter oneRewriter = new ActionsRewriter(sourceFileBuildResult.SemanticModel, sourceFileBuildResult.PrePortSemanticModel, sourceFileBuildResult.SyntaxGenerator, currentFileActions.FilePath, nodetoken.AllActions);
+
+                                    var result = root.NormalizeWhitespace().ToFullString();
+                                    //If true, line endings and spaces need to be normalized:
+                                    //if (result != root.ToFullString())
+                                    //{
+                                    //    File.WriteAllText(sourceFileBuildResult.SourceFileFullPath, result);
+                                    //}
+
+                                    var newRoot = oneRewriter.Visit(root);
+                                    var allChanges = newRoot.SyntaxTree.GetChanges(root.SyntaxTree);
+
+                                    foreach (var textChange in allChanges)
+                                    {
+                                        var fileLinePositionSpan = root.SyntaxTree.GetMappedLineSpan(textChange.Span);
+                                        var newTextChange = new TextChange() { FileLinePositionSpan = fileLinePositionSpan, NewText = textChange.NewText };
+                                        if (!nodetoken.TextChanges.Contains(newTextChange))
+                                        {
+                                            nodetoken.TextChanges.Add(newTextChange);
+                                        }
+                                    }
+                                });
+                            }


Each branch in this if...else block is quite large. What do you think about moving the logic for each branch into descriptive private functions to help with readability? For example:

if (_projectConfiguration.PortCode) { ApplyCodeActionsAndPersist( ... ); } else { // assumes TextChanges are used to preview changes ApplyCodeActionsAndPreview( ... ); }

jonlouie · 2021-04-26T06:30:07Z

src/CTA.Rules.Update/CodeReplacer.cs

+            if (_projectConfiguration.PortProject)
            {
-                var projectActionExecution = new GenericActionExecution(projectLevelAction, _projectConfiguration.ProjectPath)
+                //Project Level Actions
+                foreach (var projectLevelAction in projectActions.ProjectLevelActions)
                {
-                    TimesRun = 1
-                };
-                var runResult = string.Empty;
-                if (!_projectConfiguration.IsMockRun)
-                {
-                    if (projectLevelAction.ProjectLevelActionFunc != null)
+                    var projectActionExecution = new GenericActionExecution(projectLevelAction, _projectConfiguration.ProjectPath)
+                    {
+                        TimesRun = 1
+                    };
+                    var runResult = string.Empty;
+                    if (!_projectConfiguration.IsMockRun)
                    {
-                        try
+                        if (projectLevelAction.ProjectLevelActionFunc != null)
                        {
-                            runResult = projectLevelAction.ProjectLevelActionFunc(_projectConfiguration.ProjectPath, projectType);
+                            try
+                            {
+                                runResult = projectLevelAction.ProjectLevelActionFunc(_projectConfiguration.ProjectPath, projectType);
+                            }
+                            catch (Exception ex)
+                            {
+                                var actionExecutionException = new ActionExecutionException(projectLevelAction.Name, projectLevelAction.Key, ex);
+                                projectActionExecution.InvalidExecutions = 1;
+                                LogHelper.LogError(actionExecutionException);
+                            }
                        }
-                        catch (Exception ex)
+                        else if (projectLevelAction.ProjectFileActionFunc != null)
                        {
-                            var actionExecutionException = new ActionExecutionException(projectLevelAction.Name, projectLevelAction.Key, ex);
-                            projectActionExecution.InvalidExecutions = 1;
-                            LogHelper.LogError(actionExecutionException);
+                            try
+                            {
+                                runResult = projectLevelAction.ProjectFileActionFunc(_projectConfiguration.ProjectPath,
+                                    projectType,
+                                    _projectConfiguration.TargetVersions,
+                                    projectActions.PackageActions.Distinct().ToDictionary(p => p.Name, p => p.Version),
+                                    projectActions.ProjectReferenceActions.ToList(),
+                                    _metadataReferences);
+                            }
+                            catch (Exception ex)
+                            {
+                                var actionExecutionException = new ActionExecutionException(projectLevelAction.Name, projectLevelAction.Key, ex);
+                                projectActionExecution.InvalidExecutions = 1;
+                                LogHelper.LogError(actionExecutionException);
+                            }
                        }
                    }
-                    else if (projectLevelAction.ProjectFileActionFunc != null)
+                    if (!string.IsNullOrEmpty(runResult))
                    {
-                        try
-                        {
-                            runResult = projectLevelAction.ProjectFileActionFunc(_projectConfiguration.ProjectPath, projectType, _projectConfiguration.TargetVersions, projectActions.PackageActions.Distinct().ToDictionary(p => p.Name, p => p.Version), projectActions.ProjectReferenceActions.ToList());
-                        }
-                        catch (Exception ex)
-                        {
-                            var actionExecutionException = new ActionExecutionException(projectLevelAction.Name, projectLevelAction.Key, ex);
-                            projectActionExecution.InvalidExecutions = 1;
-                            LogHelper.LogError(actionExecutionException);
-                        }
+                        projectActionExecution.Description = string.Concat(projectActionExecution.Description, ": ", runResult);
+                        projectRunActions.Add(projectActionExecution);
+                        LogHelper.LogInformation(projectLevelAction.Description);
                    }
                }
-                if (!string.IsNullOrEmpty(runResult))
+
+                if (!actionsPerProject.TryAdd(Constants.Project, projectRunActions))
                {
-                    projectActionExecution.Description = string.Concat(projectActionExecution.Description, ": ", runResult);
-                    projectRunActions.Add(projectActionExecution);
-                    LogHelper.LogInformation(projectLevelAction.Description);
+                    LogHelper.LogError(new FilePortingException(Constants.Project, new Exception("Error adding project to actions collection")));
                }
            }


Same as above- consider moving the logic in this large if block to a private function called ApplyProjectLevelActions or something similar

jonlouie · 2021-04-26T06:42:12Z

src/CTA.Rules.Models/TextChange.cs

+
+namespace CTA.Rules.Models
+{
+    public class TextChange


A doc comment for this class would be helpful as well

Incremental and porting updates

c870a93

marknfawaz added the enhancement New feature or request label Apr 23, 2021

jonlouie suggested changes Apr 26, 2021

View reviewed changes

Review updates, separate actions for each node

7c5e0d0

huawenm approved these changes Apr 26, 2021

View reviewed changes

jonlouie approved these changes Apr 26, 2021

View reviewed changes

marknfawaz merged commit 0443de9 into main Apr 26, 2021

marknfawaz deleted the mark-incrementalfix branch April 26, 2021 22:41

This was referenced Apr 26, 2021

Update messaging in comments holding legacy metadata references #111

Open

Update how legacy metadata references are loaded based changes in CTA aws/codelyzer#70

Open


		public SolutionRewriter(IDEProjectResult projectResult, List<ProjectConfiguration> solutionConfiguration)


		public List<IDEFileActions> RunIncremental(RootNodes projectRules, List<string> updatedFiles)


		var itemGroupContent = string.Format(_itemGroupTemplate, content);

		var currentGroupTemplate = itemGroupTemplate ?? _itemGroupTemplate;

Conversation

marknfawaz commented Apr 23, 2021

Related issue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonlouie Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonlouie Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonlouie Apr 26, 2021 •

edited

Loading

jonlouie Apr 26, 2021 •

edited

Loading