Initial version of Property reads/writes analyzer

src/Build.UnitTests/Evaluation/UsedUninitializedProperties_Tests.cs

@@ -13,7 +13,7 @@ public sealed class UsedUninitializedProperties_Tests
     [Fact]
     public void Basics()
     {
-        UsedUninitializedProperties props = new();
+        PropertiesUsageTracker props = new();
 
         Assert.False(props.TryGetPropertyElementLocation("Hello", out IElementLocation? elementLocation));
         Assert.Null(elementLocation);

src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/PropertyGroupIntrinsicTask.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using Microsoft.Build.BuildCheck.Infrastructure;
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Framework;
@@ -78,14 +79,22 @@ internal override void ExecuteTask(Lookup lookup)
                                 "CannotModifyReservedProperty",
                                 property.Name);
 
-                            string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(property.Value, ExpanderOptions.ExpandAll, property.Location);
+                            bucket.Expander.PropertiesUsageTracker.CurrentlyEvaluatingPropertyElementName = property.Name;
+                            bucket.Expander.PropertiesUsageTracker.PropertyReadContext =
+                                PropertyReadContext.PropertyEvaluation;
+
+
+                            string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(property.Value, ExpanderOptions.ExpandAll, property.Location, LoggingContext);
+                            bucket.Expander.PropertiesUsageTracker.CheckPreexistingUndefinedUsage(property, evaluatedValue, LoggingContext);
 
                             if (LogTaskInputs && !LoggingContext.LoggingService.OnlyLogCriticalEvents)
                             {
                                 LoggingContext.LogComment(MessageImportance.Low, "PropertyGroupLogMessage", property.Name, evaluatedValue);
                             }
 
                             bucket.Lookup.SetProperty(ProjectPropertyInstance.Create(property.Name, evaluatedValue, property.Location, Project.IsImmutable));
+                            BuildCheckManagerProvider.GlobalBuildEngineDataConsumer.ProcessPropertyWrite(property.Name, string.IsNullOrEmpty(evaluatedValue), property.Location, LoggingContext?.BuildEventContext);
+                            bucket.Expander.PropertiesUsageTracker.ResetPropertyReadContext(false);
                         }
                     }
                 }

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Globalization;
 using System.IO;
 using System.Linq;
@@ -1225,7 +1226,8 @@ private async Task<BuildResult> BuildProject()
             {
                 buildCheckManager.EndProjectRequest(
                     BuildCheckDataSource.BuildExecution,
-                    _requestEntry.Request.ParentBuildEventContext);
+                    _requestEntry.Request.ParentBuildEventContext,
+                    _requestEntry.RequestConfiguration.ProjectFullPath);
             }
 
             BuildResult CopyTargetResultsFromProxyTargetsToRealTargets(BuildResult resultFromTargetBuilder)

src/Build/BuildCheck/API/BuildCheckResult.cs

@@ -5,6 +5,7 @@
 using System.IO;
 using Microsoft.Build.Construction;
 using Microsoft.Build.Framework;
+using Microsoft.Build.Shared;
 
 namespace Microsoft.Build.Experimental.BuildCheck;
 
@@ -16,12 +17,12 @@ namespace Microsoft.Build.Experimental.BuildCheck;
 /// </summary>
 public sealed class BuildCheckResult : IBuildCheckResult
 {
-    public static BuildCheckResult Create(BuildAnalyzerRule rule, ElementLocation location, params string[] messageArgs)
+    public static BuildCheckResult Create(BuildAnalyzerRule rule, IMsBuildElementLocation location, params string[] messageArgs)
     {
         return new BuildCheckResult(rule, location, messageArgs);
     }
 
-    public BuildCheckResult(BuildAnalyzerRule buildAnalyzerRule, ElementLocation location, string[] messageArgs)
+    public BuildCheckResult(BuildAnalyzerRule buildAnalyzerRule, IMsBuildElementLocation location, string[] messageArgs)
     {
         BuildAnalyzerRule = buildAnalyzerRule;
         Location = location;
@@ -42,7 +43,7 @@ internal BuildEventArgs ToEventArgs(BuildAnalyzerResultSeverity severity)
     /// <summary>
     /// Optional location of the finding (in near future we might need to support multiple locations).
     /// </summary>
-    public ElementLocation Location { get; }
+    public IMsBuildElementLocation Location { get; }
 
     public string LocationString => Location.LocationString;
 

src/Build/BuildCheck/API/IInternalBuildCheckRegistrationContext.cs

@@ -0,0 +1,13 @@
+using System;
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace Microsoft.Build.BuildCheck.Analyzers;
+
+internal interface IInternalBuildCheckRegistrationContext : IBuildCheckRegistrationContext
+{
+    void RegisterPropertyReadAction(Action<BuildCheckDataContext<PropertyReadData>> propertyReadAction);
+
+    void RegisterPropertyWriteAction(Action<BuildCheckDataContext<PropertyWriteData>> propertyWriteAction);
+
+    void RegisterProjectProcessingDoneAction(Action<BuildCheckDataContext<ProjectProcessingDoneData>> propertyWriteAction);
+}

src/Build/BuildCheck/API/InternalBuildAnalyzer.cs

@@ -0,0 +1,18 @@
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace Microsoft.Build.BuildCheck.Analyzers;
+
+internal abstract class InternalBuildAnalyzer : BuildAnalyzer
+{
+    /// <summary>
+    /// 
+    /// </summary>
+    /// <param name="registrationContext"></param>
+    public abstract void RegisterInternalActions(IInternalBuildCheckRegistrationContext registrationContext);
+
+    /// <summary>
+    /// This is intentionally not implemented, as it is extended by <see cref="RegisterInternalActions"/>.
+    /// </summary>
+    /// <param name="registrationContext"></param>
+    public override void RegisterActions(IBuildCheckRegistrationContext registrationContext) { }
+}

src/Build/BuildCheck/Analyzers/PropertiesUsageAnalyzer.cs

@@ -0,0 +1,154 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using Microsoft.Build.BuildCheck.Infrastructure;
+using Microsoft.Build.Collections;
+using Microsoft.Build.Evaluation;
+using Microsoft.Build.Experimental.BuildCheck;
+using Microsoft.Build.Shared;
+
+namespace Microsoft.Build.BuildCheck.Analyzers;
+
+internal class PropertiesUsageAnalyzer : InternalBuildAnalyzer
+{
+    private static readonly BuildAnalyzerRule _usedBeforeInitializedRule = new BuildAnalyzerRule("AB001", "PropertyUsedBeforeDeclared",
+        "A property that is accessed should be declared first.",
+        "Property: [{0}] was accessed, but it was never initialized.",
+        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning, IsEnabled = true, EvaluationAnalysisScope = EvaluationAnalysisScope.ProjectOnly });
+
+    private static readonly BuildAnalyzerRule _initializedAfterUsedRule = new BuildAnalyzerRule("AB002", "PropertyDeclaredAfterUsed",
+        "A property should be declared before it is first used.",
+        "Property: [{0}] first declared/initialized at [{1}] used before it was initialized.",
+        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning, IsEnabled = true, EvaluationAnalysisScope = EvaluationAnalysisScope.ProjectOnly });
+
+    private static readonly BuildAnalyzerRule _unusedPropertyRule = new BuildAnalyzerRule("AB003", "UnusedPropertyDeclared",
+        "A property that is not used should not be declared.",
+        "Property: [{0}] was declared/initialized, but it was never used.",
+        new BuildAnalyzerConfiguration() { Severity = BuildAnalyzerResultSeverity.Warning, IsEnabled = true, EvaluationAnalysisScope = EvaluationAnalysisScope.ProjectOnly });
+
+    internal static readonly IReadOnlyList<BuildAnalyzerRule> SupportedRulesList =[_usedBeforeInitializedRule, _initializedAfterUsedRule, _unusedPropertyRule];
+
+    public override string FriendlyName => "MSBuild.PropertiesUsageAnalyzer";
+
+    public override IReadOnlyList<BuildAnalyzerRule> SupportedRules => SupportedRulesList;
+
+    private const string _allowUninitPropsInConditionsKey = "AllowUninitializedPropertiesInConditions";
+    private bool _allowUninitPropsInConditions = false;
+    // TODO: Add scope to configuration visible by the analyzer - and reflect on it
+    public override void Initialize(ConfigurationContext configurationContext)
+    {
+        bool? allowUninitPropsInConditionsRule1 = null;
+        bool? allowUninitPropsInConditionsRule2 = null;
+
+        foreach (CustomConfigurationData customConfigurationData in configurationContext.CustomConfigurationData)
+        {
+            allowUninitPropsInConditionsRule1 =
+                GetAllowUninitPropsInConditionsConfig(customConfigurationData, _usedBeforeInitializedRule.Id);
+            allowUninitPropsInConditionsRule2 =
+                GetAllowUninitPropsInConditionsConfig(customConfigurationData, _initializedAfterUsedRule.Id);
+        }
+
+        if (allowUninitPropsInConditionsRule1.HasValue &&
+            allowUninitPropsInConditionsRule2.HasValue &&
+            allowUninitPropsInConditionsRule1 != allowUninitPropsInConditionsRule2)
+        {
+            throw new BuildCheckConfigurationException(
+                $"[{_usedBeforeInitializedRule.Id}] and [{_initializedAfterUsedRule.Id}] are not allowed to have differing configuration value for [{_allowUninitPropsInConditionsKey}]");
+        }
+
+        if (allowUninitPropsInConditionsRule1.HasValue || allowUninitPropsInConditionsRule2.HasValue)
+        {
+            _allowUninitPropsInConditions = allowUninitPropsInConditionsRule1 ?? allowUninitPropsInConditionsRule2 ?? false;
+        }
+    }
+
+    private static bool? GetAllowUninitPropsInConditionsConfig(CustomConfigurationData customConfigurationData,
+        string ruleId)
+    {
+        if (customConfigurationData.RuleId.Equals(ruleId, StringComparison.InvariantCultureIgnoreCase) &&
+            (customConfigurationData.ConfigurationData?.TryGetValue(_allowUninitPropsInConditionsKey, out string? configVal) ?? false))
+        {
+            return bool.Parse(configVal);
+        }
+
+        return null;
+    }
+
+    public override void RegisterInternalActions(IInternalBuildCheckRegistrationContext registrationContext)
+    {
+        registrationContext.RegisterPropertyReadAction(ProcessPropertyRead);
+        registrationContext.RegisterPropertyWriteAction(ProcessPropertyWrite);
+        registrationContext.RegisterProjectProcessingDoneAction(DoneWithProject);
+    }
+
+    private Dictionary<string, IMsBuildElementLocation?> _writenProperties = new Dictionary<string, IMsBuildElementLocation?>(MSBuildNameIgnoreCaseComparer.Default);
+    private HashSet<string> _readProperties = new HashSet<string>(MSBuildNameIgnoreCaseComparer.Default);
+    private Dictionary<string, IMsBuildElementLocation> _uninitializedReads = new Dictionary<string, IMsBuildElementLocation>(MSBuildNameIgnoreCaseComparer.Default);
+
+    private void ProcessPropertyWrite(BuildCheckDataContext<PropertyWriteData> context)
+    {
+        PropertyWriteData writeData = context.Data;
+
+        _writenProperties[writeData.PropertyName] = writeData.ElementLocation;
+
+        if (!writeData.IsEmpty && _uninitializedReads.TryGetValue(writeData.PropertyName, out IMsBuildElementLocation? uninitReadLocation))
+        {
+            _uninitializedReads.Remove(writeData.PropertyName);
+
+            context.ReportResult(BuildCheckResult.Create(
+                _initializedAfterUsedRule,
+                uninitReadLocation,
+                writeData.PropertyName, writeData.ElementLocation?.LocationString ?? string.Empty));
+        }
+    }
+
+    private void ProcessPropertyRead(BuildCheckDataContext<PropertyReadData> context)
+    {
+        PropertyReadData readData = context.Data;
+
+        if (readData.PropertyReadContext != PropertyReadContext.PropertyEvaluationSelf)
+        {
+            _readProperties.Add(readData.PropertyName);
+        }
+
+        if (readData.IsUninitialized &&
+            readData.PropertyReadContext != PropertyReadContext.PropertyEvaluationSelf &&
+            readData.PropertyReadContext != PropertyReadContext.ConditionEvaluationWithOneSideEmpty &&
+            (!_allowUninitPropsInConditions || readData.PropertyReadContext != PropertyReadContext.ConditionEvaluation))
+        {
+            _uninitializedReads[readData.PropertyName] = readData.ElementLocation;
+        }
+    }
+
+    private void DoneWithProject(BuildCheckDataContext<ProjectProcessingDoneData> context)
+    {
+        foreach (var propWithLocation in _writenProperties)
+        {
+            if (propWithLocation.Value != null && !_readProperties.Contains(propWithLocation.Key))
+            {
+                context.ReportResult(BuildCheckResult.Create(
+                    _unusedPropertyRule,
+                    propWithLocation.Value,
+                    propWithLocation.Key));
+            }
+        }
+
+        foreach (var uninitializedRead in _uninitializedReads)
+        {
+            context.ReportResult(BuildCheckResult.Create(
+                _usedBeforeInitializedRule,
+                uninitializedRead.Value,
+                uninitializedRead.Key));
+        }
+
+        _readProperties = new HashSet<string>(MSBuildNameIgnoreCaseComparer.Default);
+        _writenProperties = new Dictionary<string, IMsBuildElementLocation?>(MSBuildNameIgnoreCaseComparer.Default);
+        _uninitializedReads = new Dictionary<string, IMsBuildElementLocation>(MSBuildNameIgnoreCaseComparer.Default);
+    }
+}
+

src/Build/BuildCheck/Infrastructure/BuildCheckCentralContext.cs

@@ -6,6 +6,7 @@
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.Build.BackEnd.Logging;
+using Microsoft.Build.BuildCheck.Analyzers;
 using Microsoft.Build.Experimental.BuildCheck;
 
 namespace Microsoft.Build.BuildCheck.Infrastructure;
@@ -17,9 +18,21 @@ internal sealed class BuildCheckCentralContext
 {
     private record CallbackRegistry(
         List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<EvaluatedPropertiesAnalysisData>>)> EvaluatedPropertiesActions,
-        List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<ParsedItemsAnalysisData>>)> ParsedItemsActions)
+        List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<ParsedItemsAnalysisData>>)> ParsedItemsActions,
+        List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<PropertyReadData>>)> PropertyReadActions,
+        List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<PropertyWriteData>>)> PropertyWriteActions,
+        List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<ProjectProcessingDoneData>>)> ProjectProcessingDoneActions)
     {
-        public CallbackRegistry() : this([],[]) { }
+        public CallbackRegistry() : this([],[],[],[],[]) { }
+
+        internal void DeregisterAnalyzer(BuildAnalyzerWrapper analyzer)
+        {
+            EvaluatedPropertiesActions.RemoveAll(a => a.Item1 == analyzer);
+            ParsedItemsActions.RemoveAll(a => a.Item1 == analyzer);
+            PropertyReadActions.RemoveAll(a => a.Item1 == analyzer);
+            PropertyWriteActions.RemoveAll(a => a.Item1 == analyzer);
+            ProjectProcessingDoneActions.RemoveAll(a => a.Item1 == analyzer);
+        }
     }
 
     // In a future we can have callbacks per project as well
@@ -29,6 +42,8 @@ private record CallbackRegistry(
     //  build event args. However - this needs to be done early on, when analyzers might not be known yet
     internal bool HasEvaluatedPropertiesActions => _globalCallbacks.EvaluatedPropertiesActions.Any();
     internal bool HasParsedItemsActions => _globalCallbacks.ParsedItemsActions.Any();
+    internal bool HasPropertyReadActions => _globalCallbacks.PropertyReadActions.Any();
+    internal bool HasPropertyWriteActions => _globalCallbacks.PropertyWriteActions.Any();
 
     internal void RegisterEvaluatedPropertiesAction(BuildAnalyzerWrapper analyzer, Action<BuildCheckDataContext<EvaluatedPropertiesAnalysisData>> evaluatedPropertiesAction)
         // Here we might want to communicate to node that props need to be sent.
@@ -38,6 +53,15 @@ internal void RegisterEvaluatedPropertiesAction(BuildAnalyzerWrapper analyzer, A
     internal void RegisterParsedItemsAction(BuildAnalyzerWrapper analyzer, Action<BuildCheckDataContext<ParsedItemsAnalysisData>> parsedItemsAction)
         => RegisterAction(analyzer, parsedItemsAction, _globalCallbacks.ParsedItemsActions);
 
+    internal void RegisterPropertyReadAction(BuildAnalyzerWrapper analyzer, Action<BuildCheckDataContext<PropertyReadData>> propertyReadAction)
+        => RegisterAction(analyzer, propertyReadAction, _globalCallbacks.PropertyReadActions);
+
+    internal void RegisterPropertyWriteAction(BuildAnalyzerWrapper analyzer, Action<BuildCheckDataContext<PropertyWriteData>> propertyWriteAction)
+        => RegisterAction(analyzer, propertyWriteAction, _globalCallbacks.PropertyWriteActions);
+
+    internal void RegisterProjectProcessingDoneAction(BuildAnalyzerWrapper analyzer, Action<BuildCheckDataContext<ProjectProcessingDoneData>> projectDoneAction)
+        => RegisterAction(analyzer, projectDoneAction, _globalCallbacks.ProjectProcessingDoneActions);
+
     private void RegisterAction<T>(
         BuildAnalyzerWrapper wrappedAnalyzer,
         Action<BuildCheckDataContext<T>> handler,
@@ -58,8 +82,7 @@ void WrappedHandler(BuildCheckDataContext<T> context)
 
     internal void DeregisterAnalyzer(BuildAnalyzerWrapper analyzer)
     {
-        _globalCallbacks.EvaluatedPropertiesActions.RemoveAll(a => a.Item1 == analyzer);
-        _globalCallbacks.ParsedItemsActions.RemoveAll(a => a.Item1 == analyzer);
+        _globalCallbacks.DeregisterAnalyzer(analyzer);
     }
 
     internal void RunEvaluatedPropertiesActions(
@@ -78,6 +101,30 @@ internal void DeregisterAnalyzer(BuildAnalyzerWrapper analyzer)
         => RunRegisteredActions(_globalCallbacks.ParsedItemsActions, parsedItemsAnalysisData,
             loggingContext, resultHandler);
 
+    internal void RunPropertyReadActions(
+        PropertyReadData propertyReadDataData,
+        LoggingContext loggingContext,
+        Action<BuildAnalyzerWrapper, LoggingContext, BuildAnalyzerConfigurationInternal[], BuildCheckResult>
+            resultHandler)
+        => RunRegisteredActions(_globalCallbacks.PropertyReadActions, propertyReadDataData,
+            loggingContext, resultHandler);
+
+    internal void RunPropertyWriteActions(
+        PropertyWriteData propertyWriteData,
+        LoggingContext loggingContext,
+        Action<BuildAnalyzerWrapper, LoggingContext, BuildAnalyzerConfigurationInternal[], BuildCheckResult>
+            resultHandler)
+        => RunRegisteredActions(_globalCallbacks.PropertyWriteActions, propertyWriteData,
+            loggingContext, resultHandler);
+
+    internal void RunProjectProcessingDoneActions(
+        ProjectProcessingDoneData projectProcessingDoneData,
+        LoggingContext loggingContext,
+        Action<BuildAnalyzerWrapper, LoggingContext, BuildAnalyzerConfigurationInternal[], BuildCheckResult>
+            resultHandler)
+        => RunRegisteredActions(_globalCallbacks.ProjectProcessingDoneActions, projectProcessingDoneData,
+            loggingContext, resultHandler);
+
     private void RunRegisteredActions<T>(
         List<(BuildAnalyzerWrapper, Action<BuildCheckDataContext<T>>)> registeredCallbacks,
         T analysisData,

src/Build/BuildCheck/Infrastructure/BuildCheckConfigurationException.cs

@@ -18,4 +18,8 @@ internal sealed class BuildCheckConfigurationException : Exception
     public BuildCheckConfigurationException(string message) : base(message)
     {
     }
+
+    public BuildCheckConfigurationException(string message, Exception innerException) : base(message, innerException)
+    {
+    }
 }

src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs

@@ -56,7 +56,7 @@ private void EventSource_AnyEventRaised(object sender, BuildEventArgs e)
         }
         else if (e is ProjectFinishedEventArgs projectFinishedEventArgs)
         {
-            buildCheckManager.EndProjectRequest(BuildCheckDataSource.EventArgs, e.BuildEventContext!);
+            buildCheckManager.EndProjectRequest(BuildCheckDataSource.EventArgs, e.BuildEventContext!, projectFinishedEventArgs.ProjectFile!);
         }
         else if (e is BuildCheckEventArgs buildCheckBuildEventArgs)
         {