Initial version of properties analyzer (#10457)

* Initial version of properties analyzer

* Improve the properties analyzers

* Fix the property analyzers

* Fix suppressions

* Fix after merge

* Fix test

* Reflect PR comments

documentation/specs/BuildCheck/Codes.md

@@ -2,12 +2,14 @@
 
 Report codes are chosen to conform to suggested guidelines. Those guidelines are currently in revew: https://github.com/dotnet/msbuild/pull/10088
 
-| Exit Code | Reason |
-|:-----|----------|
-| 0 | Success |
-| [BC0101](#BC0101) | Shared output path. |
-| [BC0102](#BC0102) | Double writes. |
-| [BC0103](#BC0103) | Used environment variable. |
+| Diagnostic Code | Default Severity | Reason |
+|:-----|-------|----------|
+| [BC0101](#BC0101) | Warning | Shared output path. |
+| [BC0102](#BC0102) | Warning | Double writes. |
+| [BC0103](#BC0103) | Suggestion | Used environment variable. |
+| [BC0201](#BC0201) | Warning | Usage of undefined property. |
+| [BC0202](#BC0202) | Warning | Property first declared after it was used. |
+| [BC0203](#BC0203) | None | Property declared but never used. |
 
 
 To enable verbose logging in order to troubleshoot issue(s), enable [binary logging](https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Binary-Log.md#msbuild-binary-log-overview)
@@ -43,6 +45,48 @@ Relying on environment variables introduces variability and unpredictability, as
 
 This practice can result in inconsistent build outcomes and makes debugging difficult, since environment variables are external to project files and build scripts. To ensure consistent and reproducible builds, avoid using environment variables. Instead, explicitly pass properties using the /p option, which offers better control and traceability.
 
+## <a name="BC0201"></a>BC0201 - Usage of undefined property.
+
+"A property that is accessed should be declared first."
+
+This check indicates that a property was acessed without being declared (the declaration might have happen later - see [BC0202](#BC0202) for such checking). Only accessing in the configured scope (by default it's the project file only) are checked.
+
+There are couple cases which are allowed by the check:
+
+* Selfreferencing declaration is allowed - e.g.:
+  `<ChainProp>$(ChainProp)</ChainProp>`
+
+* Checking the property for emptyness - e.g.:
+  `<PropertyGroup Condition="'$(PropertyThatMightNotBeDefined)' == ''">`
+
+* Any usage of property in condition. This can be opted out vie the configuration `AllowUninitializedPropertiesInConditions` - e.g.:
+  ```ini
+  [*.csproj]
+  build_check.BC0201.severity=error
+  build_check.BC0201.AllowUninitializedPropertiesInConditions=false
+  build_check.BC0202.AllowUninitializedPropertiesInConditions=false
+  ```
+
+  BC0201 and BC0202 must have same value for the optional switch - as both operate on top of same data and same filtering.
+
+## <a name="BC0202"></a>BC0202 - Property first declared after it was used.
+
+"A property should be declared before it is first used."
+
+This check indicates that a property was acessed before it was declared. The default scope of this rule is the project file only. The scope captures the read and write operations as well. So this rule reports:
+ * Uninitialized reads that happened anywhere during the build, while the uninitialized property was later defined within the scope of this check (e.g. project file).
+ * Uninitialized reads that happened within the scope of check (e.g. project file), while later defined anywhere in the build
+
+If `BC0202` and [BC0201](#BC0201) are both enabled - then `BC0201` reports only the undefined reads that are not reported by this rule (so those that do not have late definitions).
+
+## <a name="BC0203"></a>BC0203 -  Property declared but never used.
+
+"A property that is not used should not be declared."
+
+This check indicates that a property was defined in the observed scope (by default it's the project file only) and it was then not used anywhere in the build.
+
+This is a runtime check, not a static analysis check - so it can have false positives (as property not used in particular build might be needed in a build with different conditions). For this reasons it's currently only suggestion.
+
 <BR/>
 <BR/>
 <BR/>

src/Build.UnitTests/BackEnd/MockLoggingService.cs

@@ -661,5 +661,8 @@ public void ProcessPropertyRead(PropertyReadInfo propertyReadInfo, CheckLoggingC
 
         public void ProcessPropertyWrite(PropertyWriteInfo propertyWriteInfo, CheckLoggingContext checkContext)
         { /* Ignore the data */ }
+
+        public void ProcessProjectEvaluationStarted(ICheckContext analysisContext, string projectFullPath)
+        { /* Ignore the data */ }
     }
 }

src/Build/BackEnd/Components/Logging/EvaluationLoggingContext.cs

@@ -3,6 +3,7 @@
 
 using System.Collections;
 using Microsoft.Build.BackEnd.Logging;
+using Microsoft.Build.Experimental.BuildCheck;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Framework.Profiler;
 using Microsoft.Build.Shared;
@@ -30,6 +31,8 @@ public EvaluationLoggingContext(ILoggingService loggingService, BuildEventContex
         public void LogProjectEvaluationStarted()
         {
             LoggingService.LogProjectEvaluationStarted(BuildEventContext, _projectFile);
+            LoggingService.BuildEngineDataRouter.ProcessProjectEvaluationStarted(
+                new CheckLoggingContext(LoggingService, BuildEventContext), _projectFile);
         }
 
         /// <summary>

src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs

@@ -803,5 +803,8 @@ public void ProcessPropertyRead(PropertyReadInfo propertyReadInfo, CheckLoggingC
 
         public void ProcessPropertyWrite(PropertyWriteInfo propertyWriteInfo, CheckLoggingContext checkContext)
             => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext);
+
+        public void ProcessProjectEvaluationStarted(ICheckContext checkContext, string projectFullPath)
+            => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath);
     }
 }

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

@@ -1125,7 +1125,7 @@ private async Task<BuildResult> BuildProject()
                 // Load the project
                 if (!_requestEntry.RequestConfiguration.IsLoaded)
                 {
-                    buildCheckManager?.StartProjectEvaluation(
+                    buildCheckManager?.ProjectFirstEncountered(
                         BuildCheckDataSource.BuildExecution,
                         new CheckLoggingContext(_nodeLoggingContext.LoggingService, _requestEntry.Request.BuildEventContext),
                         _requestEntry.RequestConfiguration.ProjectFullPath);
@@ -1151,14 +1151,12 @@ private async Task<BuildResult> BuildProject()
             finally
             {
                 buildCheckManager?.EndProjectEvaluation(
-                    BuildCheckDataSource.BuildExecution,
                     _requestEntry.Request.BuildEventContext);
             }
 
             _projectLoggingContext = _nodeLoggingContext.LogProjectStarted(_requestEntry);
             buildCheckManager?.StartProjectRequest(
-                BuildCheckDataSource.BuildExecution,
-                _requestEntry.Request.BuildEventContext,
+                _projectLoggingContext.BuildEventContext,
                 _requestEntry.RequestConfiguration.ProjectFullPath);
 
             try
@@ -1229,8 +1227,7 @@ private async Task<BuildResult> BuildProject()
             finally
             {
                 buildCheckManager?.EndProjectRequest(
-                    BuildCheckDataSource.BuildExecution,
-                    new CheckLoggingContext(_nodeLoggingContext.LoggingService, _requestEntry.Request.BuildEventContext),
+                    new CheckLoggingContext(_nodeLoggingContext.LoggingService, _projectLoggingContext.BuildEventContext),
                     _requestEntry.RequestConfiguration.ProjectFullPath);
             }
 

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(CheckRule rule, ElementLocation location, params string[] messageArgs)
+    public static BuildCheckResult Create(CheckRule rule, IMsBuildElementLocation location, params string[] messageArgs)
     {
         return new BuildCheckResult(rule, location, messageArgs);
     }
 
-    public BuildCheckResult(CheckRule checkConfig, ElementLocation location, string[] messageArgs)
+    public BuildCheckResult(CheckRule checkConfig, IMsBuildElementLocation location, string[] messageArgs)
     {
         CheckRule = checkConfig;
         Location = location;
@@ -42,7 +43,7 @@ internal BuildEventArgs ToEventArgs(CheckResultSeverity 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/IInternalCheckRegistrationContext.cs

@@ -0,0 +1,12 @@
+using System;
+
+namespace Microsoft.Build.Experimental.BuildCheck.Checks;
+
+internal interface IInternalCheckRegistrationContext : IBuildCheckRegistrationContext
+{
+    void RegisterPropertyReadAction(Action<BuildCheckDataContext<PropertyReadData>> propertyReadAction);
+
+    void RegisterPropertyWriteAction(Action<BuildCheckDataContext<PropertyWriteData>> propertyWriteAction);
+
+    void RegisterProjectRequestProcessingDoneAction(Action<BuildCheckDataContext<ProjectRequestProcessingDoneData>> propertyWriteAction);
+}

src/Build/BuildCheck/API/InternalCheck.cs

@@ -0,0 +1,28 @@
+using System;
+using Microsoft.Build.Experimental.BuildCheck;
+
+namespace Microsoft.Build.Experimental.BuildCheck.Checks;
+
+internal abstract class InternalCheck : Check
+{
+    /// <summary>
+    /// Used by the implementors to subscribe to data and events they are interested in.
+    /// This offers superset of registrations options to <see cref="Check.RegisterActions"/>.
+    /// </summary>
+    /// <param name="registrationContext"></param>
+    public abstract void RegisterInternalActions(IInternalCheckRegistrationContext 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)
+    {
+        if (registrationContext is not IInternalCheckRegistrationContext internalRegistrationContext)
+        {
+            throw new ArgumentException("The registration context for InternalBuildAnalyzer must be of type IInternalBuildCheckRegistrationContext.", nameof(registrationContext));
+        }
+
+        this.RegisterInternalActions(internalRegistrationContext);
+    }
+}