Adding support for Incompatible rules #2592

yongyan-gh · 2022-12-20T09:16:13Z

Description

Adding the IncompatibleRuleIds and IncompatibleRuleHandling properties to Skimmer according to #2591

shaopeng-gh · 2022-12-20T15:51:32Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                new IncompatibleTestRule("TEST1001", null),
+                new IncompatibleTestRule("TEST1002", null),
+                new IncompatibleTestRule("TEST1003", new HashSet<string> { "TEST1001" },  IncompatibleRuleHandling.DisableAndContinueAnalysis),
+            };


suggestion:

if rule 1 is incompatible with rule 2,
the incompatible flag can be set at rule 1, or rule 2.
And they should be "mostly" equivalent, is it correct.

What if they are both set, may add a test for that. In case both are disabled.

And then a test for rule 1 -> rule 2 -> rule 3 -> rule 1, circular case what happens.

shaopeng-gh · 2022-12-20T15:55:56Z

src/Sarif/SdkResources.resx

+  <data name="ERR998_IncompatibleRuleDetected" xml:space="preserve">
+    <value>Detected incompatible rules. Rule '{0}' is incompatible with rule '{1}'.</value>
+  </data>
+  <data name="Wrn998_IncompatibleRuleDetected" xml:space="preserve">


Wrn998

just question,
how do we choose 998 or 997, I see a few 997 above. #Resolved

"Wrn998_IncompatibleRuleDetected" should capitalize the first 3 letters to be more consistent with other names.

It is in Warnings.cs line 21, the WRN998 is the category for "// (Non-catastrophic) conditions that result in rules disabling themselves."

shaopeng-gh · 2022-12-20T16:00:41Z

src/Sarif/Warnings.cs

+                        Id = Wrn998_IncompatibleRuleDetected
+                    },
+                    Message = new Message { Text = message },
+                    Level = FailureLevel.Warning,


Level = FailureLevel.Warning,

can you add a note in the PR
why we need both error and warning,
The analysis engine could raise an error-level configuration notification and exit.

I found it: for DisableAndContinueAnalysis.

Is it something user can choose what option he wants, if so what command line parameter to pass for it:
switch (originalSkimmer.IncompatibleRuleHandling)

My understanding is that people who creates the new rules should be responsible to set the IncompatibleRuleIds and IncompatibleRuleHandling or just leave them as default.

Its not possible to set the behavior for each rule through command line right?

LingZhou-gh · 2022-12-20T19:03:15Z

src/Sarif.Driver/Sdk/IncompatibleRuleHandling.cs

+        Ignore = 0,
+        DisableAndContinueAnalysis,
+        ExitAnalysis,
+    }


The comma in "ExitAnalysis," could be removed. #Resolved

michaelcfanning · 2022-12-22T19:32:07Z

src/Sarif/RuntimeConditions.cs

@@ -52,6 +52,7 @@ public enum RuntimeConditions : uint
        ExceptionPostingLogFile = 0x40000,

        // Non-fatal conditions
+        RuleIsIncompatibleWithAnotherRule = 0x00800000,


Change this to 'OneOrMoreRulesAreIncompatible'

michaelcfanning · 2022-12-22T19:32:49Z

src/Sarif/Errors.cs

@@ -26,6 +26,7 @@ public static class Errors
        private const string ERR997_ExceptionLoadingAnalysisTarget = "ERR997.ExceptionLoadingAnalysisTarget";
        private const string ERR997_ExceptionInstantiatingSkimmers = "ERR997.ExceptionInstantiatingSkimmers";
        private const string ERR997_OutputFileAlreadyExists = "ERR997.OutputFileAlreadyExists";
+        internal const string ERR998_IncompatibleRuleDetected = "ERR997.IncompatibleRuleDetected";


ERR998_IncompatibleRuleDetected

ERR998_IncompatibleRulesDetected

Note the plural form. It takes two rules to be incompatible. :)

michaelcfanning · 2022-12-22T19:33:27Z

src/Sarif/RuntimeConditions.cs

@@ -52,6 +52,7 @@ public enum RuntimeConditions : uint
        ExceptionPostingLogFile = 0x40000,

        // Non-fatal conditions


Non

Why isn't this a fatal condition? How do we proceed if the configured rules are not compatible?

michaelcfanning · 2022-12-22T19:35:06Z

src/Sarif/Errors.cs

@@ -487,6 +488,28 @@ public static void LogUnhandledExceptionInitializingRule(IAnalysisContext contex
            context.RuntimeErrors |= RuntimeConditions.ExceptionInSkimmerAnalyze;
        }

+        public static void LogIncompatibleRule(IAnalysisContext context, string ruleId, string incompatibleRuleId)


LogIncompatibleRule

LogIncompatibleRules

Note plural form.

michaelcfanning · 2022-12-22T19:38:13Z

src/Sarif.Driver/Sdk/ExitReason.cs

@@ -11,6 +11,7 @@ public enum ExitReason
        UnhandledExceptionInstantiatingSkimmers,
        UnhandledExceptionInEngine,
        NoRulesLoaded,
+        IncompatibleRulesDetected,


IncompatibleRulesDetected

This name is good!

michaelcfanning · 2022-12-22T19:43:40Z

src/Sarif.Driver/Sdk/IncompatibleRuleHandling.cs

+
+namespace Microsoft.CodeAnalysis.Sarif.Driver
+{
+    public enum IncompatibleRuleHandling


u

You have implemented these choices cleanly. But can you explain why you think we need them? :)

If someone specifies 'ignore', then why would they bother to publish their incompatible rule ids?

Is it clear to us that a skimmer would have a single setting that would apply to all incompatible rules?

Why would a rule choose to disable and continue analysis?

What happens if two incompatible rules have different settings? If either rule chooses 'exit analysis' then we will exit. If both rules are disabled, then analysis isn't complete.

This additional machinery would be interesting if it could be specified via configuration. But if a user can specify configuration, they could simply choose to selectively enable/disable the rule they want to run.

I am interested in your thoughts, of course, but at this point I wonder whether we shouldn't simply take the clean 'exit analysis' behavior, which would be a smaller change. We could improve on it later. What's your opinion?

The reason I am thinking about adding the DisableAndContinueAnalysis is, adding a new rule with existing rule as incompatible rule should not break existing analysis.

For example: The SARIF web validator scenario. If we add the new GH1007 ProvideFullyFormattedMessageStrings, its incompatible with 'SARIF2002' ProvideMessageArguments. The validator allows users to run all SARIF* rules with all GH* rules together. After the new GH1007 rule added, users run the SARIF and GH rules together, they will get a tool configuration exception indicates there is a incompatible rule, they will not see any results. The validator needs to define 2 set of configuration files to enable SARIF2002 and disable GH1007 or the opposite, and switch configuration file between these 2. So I think an option to just disable the incompatible rule and continue analysis may be useful in this case.
What do you think

I think the exception is appropriate. When you add a new GH rule, you will be depending on existing configuration. The existing rule can't reasonably be expected to defensively turn itself off if a future rule marks it as incompatible (and what if you want the new rule to run by default, as we do with the GH analysis??).

Really, we can't handle the full matrix of configuration cases in a reasonable way, I think. Let's remove this extra complexity and just fail-fast in the event of incompatibility. For sure, we need to mark new rules with incompatibilities as a breaking change.

Please push back if you disagree and/or see deeper into this topic than I. :)

ok i agree, there are more complicated cases of handling the conflicting rules, we can start with the throwing exception now.

yongyan-gh · 2022-12-28T01:28:04Z

New PR created #2597 so close this one

yongyan-gh added 2 commits December 20, 2022 01:11

Adding support for Incompatible rules

44bea86

Fix dotnet format

70ee177

shaopeng-gh reviewed Dec 20, 2022

View reviewed changes

shaopeng-gh requested a review from suvamM December 20, 2022 16:08

LingZhou-gh reviewed Dec 20, 2022

View reviewed changes

yongyan-gh added 2 commits December 20, 2022 17:37

Fix test failures / add more test cases

499c945

Address PR feedback

5d42c7d

yongyan-gh marked this pull request as ready for review December 22, 2022 18:34

yongyan-gh requested review from michaelcfanning, EasyRhinoMSFT, marmegh and cfaucon as code owners December 22, 2022 18:34

Merge branch 'main' into users/yongyan-gh/incompatibleRules

2769694

michaelcfanning reviewed Dec 22, 2022

View reviewed changes

yongyan-gh closed this Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for Incompatible rules #2592

Adding support for Incompatible rules #2592

yongyan-gh commented Dec 20, 2022

shaopeng-gh Dec 20, 2022 •

edited

Loading

shaopeng-gh Dec 20, 2022 •

edited by yongyan-gh

Loading

LingZhou-gh Dec 20, 2022 •

edited

Loading

yongyan-gh Dec 22, 2022

shaopeng-gh Dec 20, 2022

shaopeng-gh Dec 20, 2022

shaopeng-gh Dec 20, 2022

yongyan-gh Dec 22, 2022

LingZhou-gh Dec 20, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Dec 22, 2022

michaelcfanning Dec 22, 2022

michaelcfanning Dec 22, 2022

michaelcfanning Dec 22, 2022

michaelcfanning Dec 22, 2022

michaelcfanning Dec 22, 2022

yongyan-gh Dec 22, 2022 •

edited

Loading

michaelcfanning Dec 22, 2022

yongyan-gh Dec 23, 2022

yongyan-gh commented Dec 28, 2022

		@@ -52,6 +52,7 @@ public enum RuntimeConditions : uint
		ExceptionPostingLogFile = 0x40000,

		// Non-fatal conditions

Adding support for Incompatible rules #2592

Adding support for Incompatible rules #2592

Conversation

yongyan-gh commented Dec 20, 2022

Description

shaopeng-gh Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh Dec 20, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

LingZhou-gh Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LingZhou-gh Dec 20, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh commented Dec 28, 2022

shaopeng-gh Dec 20, 2022 •

edited

Loading

shaopeng-gh Dec 20, 2022 •

edited by yongyan-gh

Loading

LingZhou-gh Dec 20, 2022 •

edited

Loading

LingZhou-gh Dec 20, 2022 •

edited by yongyan-gh

Loading

yongyan-gh Dec 22, 2022 •

edited

Loading