Extract EnvVariable check invocation from ProcessEvaluationFinishedEv…

…entArgs + add Build check events to binglog (#10502)

src/Build/BuildCheck/API/BuildCheckResult.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using System.IO;
 using Microsoft.Build.Construction;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
@@ -13,14 +12,11 @@ namespace Microsoft.Build.Experimental.BuildCheck;
 /// Representation of a single report of a single finding from a Check
 /// Each rule has upfront known message format - so only the concrete arguments are added
 /// Optionally a location is attached - in the near future we might need to support multiple locations
-///  (for 2 cases - a) grouped result for multiple occurrences; b) a single report for a finding resulting from combination of multiple locations)
+///  (for 2 cases - a) grouped result for multiple occurrences; b) a single report for a finding resulting from combination of multiple locations).
 /// </summary>
 public sealed class BuildCheckResult : IBuildCheckResult
 {
-    public static BuildCheckResult Create(CheckRule rule, IMSBuildElementLocation location, params string[] messageArgs)
-    {
-        return new BuildCheckResult(rule, location, messageArgs);
-    }
+    public static BuildCheckResult Create(CheckRule rule, IMSBuildElementLocation location, params string[] messageArgs) => new BuildCheckResult(rule, location, messageArgs);
 
     public BuildCheckResult(CheckRule checkConfig, IMSBuildElementLocation location, string[] messageArgs)
     {
@@ -48,6 +44,7 @@ internal BuildEventArgs ToEventArgs(CheckResultSeverity severity)
     public string LocationString => Location.LocationString;
 
     public string[] MessageArgs { get; }
+
     public string MessageFormat => CheckRule.MessageFormat;
 
     // Here we will provide different link for built-in rules and custom rules - once we have the base classes differentiated.

src/Build/BuildCheck/API/CheckConfiguration.cs

@@ -73,7 +73,6 @@ public bool? IsEnabled {
         Severity = TryExtractSeverity(configDictionary),
     };
 
-
     private static EvaluationCheckScope? TryExtractEvaluationCheckScope(Dictionary<string, string>? config)
     {
 

src/Build/BuildCheck/API/IBuildCheckRegistrationContext.cs

@@ -13,5 +13,7 @@ public interface IBuildCheckRegistrationContext
 
     void RegisterTaskInvocationAction(Action<BuildCheckDataContext<TaskInvocationCheckData>> taskInvocationAction);
 
+    void RegisterEnvironmentVariableReadAction(Action<BuildCheckDataContext<EnvironmentVariableCheckData>> environmentVariableAction);
+
     void RegisterBuildFinishedAction(Action<BuildCheckDataContext<BuildFinishedCheckData>> buildFinishedAction);
 }

src/Build/BuildCheck/Checks/NoEnvironmentVariablePropertyCheck.cs

@@ -4,27 +4,29 @@
 using System;
 using System.Collections.Generic;
 using Microsoft.Build.BuildCheck.Infrastructure;
-using Microsoft.Build.Construction;
+using Microsoft.Build.Shared;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Checks;
 
 internal sealed class NoEnvironmentVariablePropertyCheck : Check
 {
     public static CheckRule SupportedRule = new CheckRule(
-                "BC0103",
-                "NoEnvironmentVariablePropertyCheck",
-                "No implicit property derived from an environment variable should be used during the build",
-                "Property is derived from environment variable: {0}. Properties should be passed explicitly using the /p option.",
-                new CheckConfiguration() { Severity = CheckResultSeverity.Suggestion });
+        "BC0103",
+        "NoEnvironmentVariablePropertyCheck",
+        "No implicit property derived from an environment variable should be used during the build",
+        "Property is derived from environment variable: {0}. Properties should be passed explicitly using the /p option.",
+        new CheckConfiguration() { Severity = CheckResultSeverity.Suggestion });
 
     private const string RuleId = "BC0103";
 
     private const string VerboseEnvVariableOutputKey = "allow_displaying_environment_variable_value";
 
+    private readonly Queue<(string projectPath, BuildCheckDataContext<EnvironmentVariableCheckData>)> _buildCheckResults = new Queue<(string, BuildCheckDataContext<EnvironmentVariableCheckData>)>();
+
     /// <summary>
-    /// Contains the list of reported environment variables.
+    /// Contains the list of viewed environment variables.
     /// </summary>
-    private readonly HashSet<EnvironmentVariableIdentityKey> _environmentVariablesReported = new HashSet<EnvironmentVariableIdentityKey>();
+    private readonly HashSet<EnvironmentVariableIdentityKey> _environmentVariablesCache = new HashSet<EnvironmentVariableIdentityKey>();
 
     private bool _isVerboseEnvVarOutput;
     private EvaluationCheckScope _scope;
@@ -39,44 +41,33 @@ public override void Initialize(ConfigurationContext configurationContext)
         foreach (CustomConfigurationData customConfigurationData in configurationContext.CustomConfigurationData)
         {
             bool? isVerboseEnvVarOutput = GetVerboseEnvVarOutputConfig(customConfigurationData, RuleId);
-            _isVerboseEnvVarOutput = isVerboseEnvVarOutput.HasValue && isVerboseEnvVarOutput.Value;           
+            _isVerboseEnvVarOutput = isVerboseEnvVarOutput.HasValue && isVerboseEnvVarOutput.Value;
         }
+
+        CheckScopeClassifier.NotifyOnScopingReadiness += HandleScopeReadiness;
     }
 
-    public override void RegisterActions(IBuildCheckRegistrationContext registrationContext) => registrationContext.RegisterEvaluatedPropertiesAction(ProcessEnvironmentVariableReadAction);
+    public override void RegisterActions(IBuildCheckRegistrationContext registrationContext) => registrationContext.RegisterEnvironmentVariableReadAction(ProcessEnvironmentVariableReadAction);
 
-    private void ProcessEnvironmentVariableReadAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
+    private void ProcessEnvironmentVariableReadAction(BuildCheckDataContext<EnvironmentVariableCheckData> context)
     {
-        if (context.Data.EvaluatedEnvironmentVariables.Count != 0)
+        EnvironmentVariableIdentityKey identityKey = new(context.Data.EnvironmentVariableName, context.Data.EnvironmentVariableLocation);
+        if (!_environmentVariablesCache.Contains(identityKey))
         {
-            foreach (var envVariableData in context.Data.EvaluatedEnvironmentVariables)
+            // Scope information is available after evaluation of the project file. If it is not ready, we will report the check later.
+            if (!CheckScopeClassifier.IsScopingReady(_scope))
+            {
+                _buildCheckResults.Enqueue((context.Data.ProjectFilePath, context));
+            }
+            else if (CheckScopeClassifier.IsActionInObservedScope(_scope, context.Data.EnvironmentVariableLocation.File, context.Data.ProjectFilePath))
             {
-                if (!CheckScopeClassifier.IsActionInObservedScope(_scope, envVariableData.Value.File,
-                        context.Data.ProjectFilePath))
-                {
-                    continue;
-                }
-                EnvironmentVariableIdentityKey identityKey = new(envVariableData.Key, envVariableData.Value.File, envVariableData.Value.Line, envVariableData.Value.Column);
-                if (!_environmentVariablesReported.Contains(identityKey))
-                {
-                    if (_isVerboseEnvVarOutput)
-                    {
-                        context.ReportResult(BuildCheckResult.Create(
-                            SupportedRule,
-                            ElementLocation.Create(envVariableData.Value.File, envVariableData.Value.Line, envVariableData.Value.Column),
-                            $"'{envVariableData.Key}' with value: '{envVariableData.Value.EnvVarValue}'"));
-                    }
-                    else
-                    {
-                        context.ReportResult(BuildCheckResult.Create(
-                            SupportedRule,
-                            ElementLocation.Create(envVariableData.Value.File, envVariableData.Value.Line, envVariableData.Value.Column),
-                            $"'{envVariableData.Key}'"));
-                    }
-
-                    _environmentVariablesReported.Add(identityKey);
-                }
+                context.ReportResult(BuildCheckResult.Create(
+                    SupportedRule,
+                    context.Data.EnvironmentVariableLocation,
+                    GetFormattedMessage(context.Data.EnvironmentVariableName, context.Data.EnvironmentVariableValue)));
             }
+
+            _environmentVariablesCache.Add(identityKey);
         }
     }
 
@@ -85,31 +76,49 @@ private void ProcessEnvironmentVariableReadAction(BuildCheckDataContext<Evaluate
             ? bool.Parse(configVal)
             : null;
 
-    internal class EnvironmentVariableIdentityKey(string environmentVariableName, string file, int line, int column) : IEquatable<EnvironmentVariableIdentityKey>
+    private void HandleScopeReadiness()
     {
-        public string EnvironmentVariableName { get; } = environmentVariableName;
+        while (_buildCheckResults.Count > 0)
+        {
+            (string projectPath, BuildCheckDataContext<EnvironmentVariableCheckData> context) = _buildCheckResults.Dequeue();
+            if (!CheckScopeClassifier.IsActionInObservedScope(_scope, context.Data.EnvironmentVariableLocation.File, projectPath))
+            {
+                continue;
+            }
+
+            context.ReportResult(BuildCheckResult.Create(
+                SupportedRule,
+                context.Data.EnvironmentVariableLocation,
+                GetFormattedMessage(context.Data.EnvironmentVariableName, context.Data.EnvironmentVariableValue)));
+        }
+
+        CheckScopeClassifier.NotifyOnScopingReadiness -= HandleScopeReadiness;
+    }
 
-        public string File { get; } = file;
+    private string GetFormattedMessage(string envVariableName, string envVariableValue) => _isVerboseEnvVarOutput? $"'{envVariableName}' with value: '{envVariableValue}'" : $"'{envVariableName}'";
 
-        public int Line { get; } = line;
+    internal class EnvironmentVariableIdentityKey(string environmentVariableName, IMSBuildElementLocation location) : IEquatable<EnvironmentVariableIdentityKey>
+    {
+        public string EnvironmentVariableName { get; } = environmentVariableName;
 
-        public int Column { get; } = column;
+        public IMSBuildElementLocation Location { get; } = location;
 
         public override bool Equals(object? obj) => Equals(obj as EnvironmentVariableIdentityKey);
 
         public bool Equals(EnvironmentVariableIdentityKey? other) =>
             other != null &&
             EnvironmentVariableName == other.EnvironmentVariableName &&
-            File == other.File &&
-            Line == other.Line &&
-            Column == other.Column;
+            Location.File == other.Location.File &&
+            Location.Line == other.Location.Line &&
+            Location.Column == other.Location.Column;
 
         public override int GetHashCode()
         {
             int hashCode = 17;
-            hashCode = hashCode * 31 + (File != null ? File.GetHashCode() : 0);
-            hashCode = hashCode * 31 + Line.GetHashCode();
-            hashCode = hashCode * 31 + Column.GetHashCode();
+            hashCode = hashCode * 31 + (Location.File != null ? Location.File.GetHashCode() : 0);
+            hashCode = hashCode * 31 + Location.Line.GetHashCode();
+            hashCode = hashCode * 31 + Location.Column.GetHashCode();
+
             return hashCode;
         }
     }

src/Build/BuildCheck/Infrastructure/BuildCheckCentralContext.cs

@@ -4,10 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using System.Threading.Tasks;
-using Microsoft.Build.BackEnd.Logging;
 using Microsoft.Build.BuildCheck.Infrastructure;
-using Microsoft.Build.Experimental.BuildCheck;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 
@@ -28,9 +25,13 @@ private record CallbackRegistry(
         List<(CheckWrapper, Action<BuildCheckDataContext<PropertyReadData>>)> PropertyReadActions,
         List<(CheckWrapper, Action<BuildCheckDataContext<PropertyWriteData>>)> PropertyWriteActions,
         List<(CheckWrapper, Action<BuildCheckDataContext<ProjectRequestProcessingDoneData>>)> ProjectRequestProcessingDoneActions,
-        List<(CheckWrapper, Action<BuildCheckDataContext<BuildFinishedCheckData>>)> BuildFinishedActions)
+        List<(CheckWrapper, Action<BuildCheckDataContext<BuildFinishedCheckData>>)> BuildFinishedActions,
+        List<(CheckWrapper, Action<BuildCheckDataContext<EnvironmentVariableCheckData>>)> EnvironmentVariableCheckDataActions)
     {
-        public CallbackRegistry() : this([], [], [], [], [], [], []) { }
+        public CallbackRegistry()
+            : this([], [], [], [], [], [], [], [])
+        {
+        }
 
         internal void DeregisterCheck(CheckWrapper check)
         {
@@ -53,10 +54,15 @@ internal void DeregisterCheck(CheckWrapper check)
     internal bool HasParsedItemsActions => _globalCallbacks.ParsedItemsActions.Count > 0;
 
     internal bool HasTaskInvocationActions => _globalCallbacks.TaskInvocationActions.Count > 0;
+
     internal bool HasPropertyReadActions => _globalCallbacks.PropertyReadActions.Count > 0;
+
     internal bool HasPropertyWriteActions => _globalCallbacks.PropertyWriteActions.Count > 0;
     internal bool HasBuildFinishedActions => _globalCallbacks.BuildFinishedActions.Count > 0;
 
+    internal void RegisterEnvironmentVariableReadAction(CheckWrapper check, Action<BuildCheckDataContext<EnvironmentVariableCheckData>> environmentVariableAction)
+       => RegisterAction(check, environmentVariableAction, _globalCallbacks.EnvironmentVariableCheckDataActions);
+
     internal void RegisterEvaluatedPropertiesAction(CheckWrapper check, Action<BuildCheckDataContext<EvaluatedPropertiesCheckData>> evaluatedPropertiesAction)
         // Here we might want to communicate to node that props need to be sent.
         //  (it was being communicated via MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION)
@@ -98,18 +104,21 @@ void WrappedHandler(BuildCheckDataContext<T> context)
         }
     }
 
-    internal void DeregisterCheck(CheckWrapper check)
-    {
-        _globalCallbacks.DeregisterCheck(check);
-    }
+    internal void DeregisterCheck(CheckWrapper check) => _globalCallbacks.DeregisterCheck(check);
+
+    internal void RunEnvironmentVariableActions(
+        EnvironmentVariableCheckData environmentVariableCheckData,
+        ICheckContext checkContext,
+        Action<CheckWrapper, ICheckContext, CheckConfigurationEffective[], BuildCheckResult>
+            resultHandler)
+        => RunRegisteredActions(_globalCallbacks.EnvironmentVariableCheckDataActions, environmentVariableCheckData, checkContext, resultHandler);
 
     internal void RunEvaluatedPropertiesActions(
         EvaluatedPropertiesCheckData evaluatedPropertiesCheckData,
         ICheckContext checkContext,
         Action<CheckWrapper, ICheckContext, CheckConfigurationEffective[], BuildCheckResult>
             resultHandler)
-        => RunRegisteredActions(_globalCallbacks.EvaluatedPropertiesActions, evaluatedPropertiesCheckData,
-            checkContext, resultHandler);
+        => RunRegisteredActions(_globalCallbacks.EvaluatedPropertiesActions, evaluatedPropertiesCheckData, checkContext, resultHandler);
 
     internal void RunParsedItemsActions(
         ParsedItemsCheckData parsedItemsCheckData,
@@ -187,9 +196,7 @@ private void RunRegisteredActions<T>(
             }
             else
             {
-                configPerRule =
-                    _configurationProvider.GetMergedConfigurations(projectFullPath,
-                        checkCallback.Item1.Check);
+                configPerRule = _configurationProvider.GetMergedConfigurations(projectFullPath, checkCallback.Item1.Check);
                 if (configPerRule.All(c => !c.IsEnabled))
                 {
                     return;

src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs

@@ -4,18 +4,17 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
-using System.Diagnostics;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.BackEnd.Logging;
-using Microsoft.Build.Experimental.BuildCheck;
+using Microsoft.Build.BuildCheck.Infrastructure;
+using Microsoft.Build.Construction;
 using Microsoft.Build.Experimental.BuildCheck.Acquisition;
 using Microsoft.Build.Experimental.BuildCheck.Checks;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
-using Microsoft.Build.Evaluation;
-using Microsoft.Build.BuildCheck.Infrastructure;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 
@@ -324,6 +323,7 @@ public void ProcessEvaluationFinishedEventArgs(
             ProjectEvaluationFinishedEventArgs evaluationFinishedEventArgs)
         {
             Dictionary<string, string>? propertiesLookup = null;
+
             // The FileClassifier is normally initialized by executing build requests.
             // However, if we are running in a main node that has no execution nodes - we need to initialize it here (from events).
             if (!IsInProcNode)
@@ -345,12 +345,15 @@ public void ProcessEnvironmentVariableReadEventArgs(ICheckContext checkContext,
         {
             if (projectEvaluationEventArgs is EnvironmentVariableReadEventArgs evr)
             {
-                _buildEventsProcessor.ProcessEnvironmentVariableReadEventArgs(
-                    evr.EnvironmentVariableName,
-                    evr.Message ?? string.Empty,
-                    evr.File,
-                    evr.LineNumber,
-                    evr.ColumnNumber);
+                if (TryGetProjectFullPath(checkContext.BuildEventContext, out string projectPath))
+                {
+                    _buildEventsProcessor.ProcessEnvironmentVariableReadEventArgs(
+                        checkContext,
+                        projectPath,
+                        evr.EnvironmentVariableName,
+                        evr.Message ?? string.Empty,
+                        ElementLocation.Create(evr.File, evr.LineNumber, evr.ColumnNumber));
+                }
             }
         }
 
@@ -405,7 +408,7 @@ public void FinalizeProcessing(LoggingContext loggingContext)
             loggingContext.LogBuildEvent(checkEventArg);
         }
 
-        private readonly ConcurrentDictionary<int, string> _projectsByInstnaceId = new();
+        private readonly ConcurrentDictionary<int, string> _projectsByInstanceId = new();
         private readonly ConcurrentDictionary<int, string> _projectsByEvaluationId = new();
 
         /// <summary>
@@ -428,15 +431,15 @@ private bool TryGetProjectFullPath(BuildEventContext buildEventContext, out stri
             }
             else if (buildEventContext.ProjectInstanceId >= 0)
             {
-                if (_projectsByInstnaceId.TryGetValue(buildEventContext.ProjectInstanceId, out string? val))
+                if (_projectsByInstanceId.TryGetValue(buildEventContext.ProjectInstanceId, out string? val))
                 {
                     projectFullPath = val;
                     return true;
                 }
             }
-            else if (_projectsByInstnaceId.Count == 1)
+            else if (_projectsByInstanceId.Count == 1)
             {
-                projectFullPath = _projectsByInstnaceId.FirstOrDefault().Value;
+                projectFullPath = _projectsByInstanceId.FirstOrDefault().Value;
                 // This is for a rare possibility of a race where other thread removed the item (between the if check and fetch here).
                 // We currently do not support multiple projects in parallel in a single node anyway.
                 if (!string.IsNullOrEmpty(projectFullPath))
@@ -494,7 +497,7 @@ public void EndProjectEvaluation(BuildEventContext buildEventContext)
         public void StartProjectRequest(BuildEventContext buildEventContext, string projectFullPath)
         {
             // There can be multiple ProjectStarted-ProjectFinished per single configuration project build (each request for different target)
-            _projectsByInstnaceId[buildEventContext.ProjectInstanceId] = projectFullPath;
+            _projectsByInstanceId[buildEventContext.ProjectInstanceId] = projectFullPath;
         }
 
         public void EndProjectRequest(