Bring /errorlog output up to SARIF draft 1.0.0-beta.5 #11233

nguerrera · 2016-05-11T00:59:30Z

Bring /errorlog output up to SARIF draft 1.0.0-beta.5

Also fix #9769

Stop escaping forward slashes (optional per spec and users don't want it)
Add tests converted from DataContractSerializer tests that stress the code we borrowed from there for escaping.
3 .Let the underlying TextWriter handle all newlines

@dotnet/roslyn-compiler @mavasani @lgolding @michaelcfanning

michaelcfanning · 2016-05-12T00:18:16Z

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs

@@ -45,7 +47,7 @@ public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Versi

        private void WriteToolInfo(string name, string fileVersion, Version assemblyVersion)
        {
-            _writer.WriteObjectStart("toolInfo");
+            _writer.WriteObjectStart("tool");
            _writer.Write("name", name);
            _writer.Write("version", assemblyVersion.ToString(fieldCount: 3));


The convention I've followed in the SARIF SDK is to emit the four-part assembly version to the 'version' field. Then emit the three-part version as the 'semanticVersion' field. Then, we emit the assembly file version but only if it differs from the assembly version.

you should consider emitting a marker in the sarif file that provides an explicit version for your SARIF producer. in the SDK, we dump a custom property SarifLoggerVersion on the tools property. this isn't necessary, though, if other version information sufficiently identifies this specific point-in-time (perhaps your assembly file version suffices)?

@lgolding, our tolerance for additions to SARIF is very low. you could imagine, though, extending the 'tool' metadata to include a 'sarifLoggerVersion' for tools that integrate some other component for producing SARIF (that may be versioned separately from the root tool itself). i.e., promote the SDK custom property.

re 'semanticVersion', that's new, eh? I didn't see it. The way we write it now was conformant to prior spec iteration where 'version' was supposed to be semantic. I'll do it as you suggest.

There are two issues:

Should there be a toolInfo.loggerVersion property?

If there were, would Nick use it here?

If the answer to #2 is "no", we can defer #1 to future.

So... why did you think Nick would use a loggerVersion property? Your scenario was " tools that integrate some other component for producing SARIF ". But Nick isn't using the SARIF SDK; the logging code is built into Roslyn. Why doesn't the version of the Roslyn compiler suffice?

In reply to: 62950703 [](ancestors = 62950703)

ok, to close. Nick will update version emitted to conform to my guidance (we have tracking items to update the SARIF spec). we are adding a 'sarifLoggerVersion' member that can be populated if the sarif-producing component is versioned separately from the dependent tool. Nick doesn't need to be concerned about populating this item, as the Roslyn version uniquely identifies the sarif-producing code.

michaelcfanning · 2016-05-12T00:47:08Z

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs

@@ -35,8 +37,8 @@ public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Versi
            _writer.WriteObjectStart(); // root
            _writer.Write("version", OutputFormatVersion);



if you emit a $schema property, your emitted SARIF will automatically validate when opened in VS or VS Code.

"$schema": "http://json.schemastore.org/sarif-1.0.0"

I can emit this, but I'll wait until that URL does not have anything beta in it and it actually has the final schema. If we ship U3 against a spec still in beta then beta -> RTM changes will cause U3 logs to fail to validate. I know the plan is not for that to happen, but I'll be conservative and add $schema only when we change 'version' to '1.0.0' in the output.

nguerrera · 2016-05-12T00:54:40Z

I pushed a new commit with rule metadata implemented. There are two tests failing but I need to run soon and wanted to show active reviewers latest code.I will fix the tests and the PR feedback in the morning.

ghost · 2016-05-12T14:58:11Z

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs

-                _writer.Write("shortMessage", message);
-                _writer.Write("fullMessage", description);
+                _writer.WriteArrayStart("suppressionStates");
+                _writer.Write("suppressedInSource");


@michaelfanning, This is correct according to the schema as it stands today but we talked about splitting SuppressionState and BaselineState apart, and I see we never did that. @nguerrera, this is fine for now but there will be a small change. I'll file an issue in the SDK to take care of this.

Oh! Red alert! Two things:

@michaelcfanning: The schema correctly shows the values of the SuppressionStates enumeration as "suppressed", "suppressedInSource". But the code gen hints file gives the enumeration constant names as "SuppressedInSource", "SuppressedInBaseline", so the generated file SuppressionStates.cs is wrong. I'll fix that.

@nguerrera: The interpretation of the SuppressionStates enum is subtle. Every result that is suppressed, for whatever reason, should have the value "suppressed" in the "suppressionStates" array. Those results that are suppressed because of an in-source suppression should also have the value "suppressedInSource" in the array. I can explain offline the reasoning behind that.

So, since in Roslyn every suppression you see is a source-level suppression (that's right, isn't it?) your code should be

_writer.WriteArrayStart("suppressionStates"); _writer.Write("suppressed"); _writer.Write("suppressedInSource");

... resulting in

"suppressionStates": [ "suppressed", "suppressedInSource" ]

In reply to: 63035326 [](ancestors = 63035326)

This is what I see in http://json.schemastore.org/sarifright now. Is it not up to date with what is supposed to be done here?

"suppressionStates": { "type": "array", "items": { "description": "A flag value indicating one or more suppression conditions.", "enum": [ "suppressedInSource", "suppressedInBaseline" ] } },

OK, I found bleeding edge in https://github.com/Microsoft/sarif-sdk/blob/master/src/Sarif/Schemata/Sarif.schema.json and will adjust to it. Is it wise that we are keeping beta.4 version across breaking changes? Isn't this exactly what Ryley was plagued by?

It is not. We should have changed to beta.5 when we made that change.

In reply to: 63069504 [](ancestors = 63069504)

It's not too late since the latest schema hasn't been published. Should I bump to beta.5 in next commit where I do this and adjust tags to be under properties?

Yes. I'm working on the commit now.

In reply to: 63072622 [](ancestors = 63072622)

Offline, we decided it should remain [ "suppressedInSource" ]

ghost · 2016-05-12T15:38:27Z

ghost · 2016-05-12T15:39:51Z

👍 subject to the [ "suppressed", "suppressedInSource" ] change.

mavasani · 2016-05-12T15:44:05Z

This will probably break errorlog VSI test

mavasani · 2016-05-12T15:44:24Z

test vsi please

nguerrera · 2016-05-12T17:35:58Z

@mavasani where do the relevant vsi tests live and hoiw do I run them?

mavasani · 2016-05-12T17:39:16Z

@nguerrera The tests live in the roslyn-internal repo. We have a test for diagnostics and FixAll occurrences, both of which verify expected diagnostics against checked in error log baselines. You will basically need to generate new error log baselines by manually running the test scenario. We can do it together, I'll drop by after standup.

mavasani · 2016-05-12T17:41:29Z

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs


-        public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Version toolAssemblyVersion)
+        public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Version toolAssemblyVersion, CultureInfo culture)


I'd probably just add a separate constructor - even though this is not public API, I know of a bunch of tools that use reflection to instantiate an error logger. We can avoid breaking them..

nguerrera · 2016-05-12T21:22:48Z

@lgolding @michaelcfanning Everything should be up to date with discussion this afternoon and pending beta.5. I've rebased my other active PR (and closed it) in to this one since I needed the escaping fix to test the $schema value that is now written.

Not ready to merge as I still need to:

Add some test coverage for additional locations, multiple descriptors with same ID.
Refine the DescriptorSet to handle a pathological case I thought about last night (e.g. one descriptor has ID XXX.0001 and two others have XXX)
Work with @mavasani to get vsi tests rebaselined.

JSON spec makes escaping of forward slashes optional and many have complained about the aesthetics of '\/'. Previously, we got this behavior from the DataContractJsonSerializer and could not override it, but now that the dependency has been removed, we can eliminate the unnecessary escaping.

Also, defer to TextWriter for NewLine character rather than hard-coding Environment.NewLine.

Also remove outdated future improvement comment

nguerrera · 2016-05-13T21:48:57Z

I believe all feedback has been addressed, except I still need @mavasani's help to rebaseline vsi tests.

jmarolf · 2016-05-15T04:25:32Z

test vsi please

mavasani · 2016-05-16T16:08:35Z

@nguerrera I am going to skip the error log based tests to allow your checkin through, then regenerate the baseline, checkin a text file with steps to regenerate it in future and also modify the error log tao actions to throw an appropriate error pointing out the need for baseline update.

nguerrera · 2016-05-16T17:33:17Z

@mavasani OK. Thanks!

mavasani · 2016-05-16T17:51:11Z

@dotnet-bot retest roslyn_prtest_win_vsi2 please

mavasani · 2016-05-16T17:51:52Z

test vsi please

mavasani · 2016-05-16T18:53:53Z

@nguerrera Tests are now all green - I'll fix and unskip them once your change is merged in.

nguerrera · 2016-05-16T20:24:11Z

Reviewers: is this ready to go? I need to get this in today. @lgolding @mavasani @jaredpar

ghost · 2016-05-16T20:26:45Z

+1 from me. I didn't have any real feedback in the first place.

mavasani · 2016-05-16T20:29:59Z

docs/compilers/Error Log Format.md


 Key                      | Value
 ------------------------ | ------------
-"severity"               | `Diagnostic.Severity` ("Hidden", "Info", "Warning, or "Error")


Why are we getting rid of severity?

Everything removed has been replaced with first class SARIF equivalents.

mavasani · 2016-05-16T20:39:24Z

LGTM

mavasani · 2016-05-17T16:16:54Z

@nguerrera Are you planning to check this in today?

nguerrera · 2016-05-17T17:06:04Z

@mavasani Yes. I've been waiting on at least one compiler team reviewer. @agocke @jcouv

agocke · 2016-05-17T17:08:11Z

src/Compilers/Core/Portable/InternalUtilities/JsonWriter.cs

        }

        public void Write(int value)
        {
            WritePending();
-            _output.Write(value);
-            _pending = s_commaNewLine;
+            _output.Write(value.ToString(CultureInfo.InvariantCulture));


Is this spec'd somewhere? The invariant culture is pretty strange and not common outside of .NET

agocke · 2016-05-17T17:12:46Z

LGTM aside from invariant culture. It looks like you're only using it for integers and characters, so it should probably be safe, but if we ever handle something like dates I would prefer ISO 8601 or a similar standard.

ghost · 2016-05-17T17:20:28Z

@agocke The log files are intended to be machine-readable so use of InvariantCulture is appropriate here.

As for dates, the SARIF standard specifies a particular ISO 8601 date format (link).

@nguerrera Please be sure you are emitting this format if you are populating any of the following properties:

invocation.startTime
invocation.endTime
notification.time

Those are all optional properties, and you're probably not populating any of them, especially if you wanted the log file to be deterministic.

nguerrera · 2016-05-17T17:22:47Z

@agocke. Thanks. CultureInfo.InvariantCulture is used exclusively for formatting integers. That part of the change is defending against a CurrentCulture that would not format integers compatibly with JSON.

"8 Numbers" in http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf defines the number format and I added tests coverage: https://github.com/dotnet/roslyn/pull/11233/files#diff-5bdd83988167258d8851cb9093c1cf25R32

@lgolding I don't emit any of those at this time.

mavasani · 2016-05-17T21:54:33Z

@nguerrera I am trying to update the error log baseline and error log tao action that parses the error log file and I see that the error log has duplicate rules metadata entries like below:

        "SA1003": {
          "id": "SA1003",
          "shortDescription": "Symbols must be spaced correctly",
          "fullDescription": "The spacing around an operator symbol is incorrect, within a C# code file.",
          "defaultLevel": "warning",
          "helpUri": "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1003.md",
          "properties": {
            "category": "StyleCop.CSharp.SpacingRules",
            "isEnabledByDefault": true
          }
        },
        "SA1003.001": {
          "id": "SA1003",
          "shortDescription": "Symbols must be spaced correctly",
          "fullDescription": "The spacing around an operator symbol is incorrect, within a C# code file.",
          "defaultLevel": "warning",
          "helpUri": "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1003.md",
          "properties": {
            "category": "StyleCop.CSharp.SpacingRules",
            "isEnabledByDefault": true
          }
        }

This causes a "ruleKey" entry to be emitted for individual diagnostics which is causing intermittent failure. For now, I am going to work around this and file a separate bug for this issue.

mavasani · 2016-05-17T22:12:40Z

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs

+            private Dictionary<string, int> _counters = new Dictionary<string, int>();
+
+            // DiagnosticDescriptor -> unique key
+            private Dictionary<DiagnosticDescriptor, string> _keys = new Dictionary<DiagnosticDescriptor, string>();


You probably need to use a DiagnosticDescriptor comparer that ensures equivalent descriptors which are not reference equals don't cause duplicate kvps.

DiagnosticDescriptor is IEquatable<DiagnosticDescriptor> by value, so that's why I thought default comparer would work. However, I see now that common case of unequal descriptors with same ID is when they differ only by message format and we don't write out the message format. So I do need a comparer that only compares the parts we write out. On it.

mavasani · 2016-05-17T22:47:16Z

@nguerrera I have filed #11383 and assigned it to you.

dnfclas added the cla-already-signed label May 11, 2016

michaelcfanning reviewed May 12, 2016
View reviewed changes

nguerrera changed the title ~~Bring /errorlog output up to SARIF draft 1.0.0-beta.4~~ [WIP] Bring /errorlog output up to SARIF draft 1.0.0-beta.4 May 12, 2016

michaelcfanning reviewed May 12, 2016
View reviewed changes

ghost reviewed May 12, 2016
View reviewed changes

mavasani reviewed May 12, 2016
View reviewed changes

nguerrera force-pushed the sarif-update branch 2 times, most recently from 51fb515 to 9f5a4ff Compare May 12, 2016 21:15

nguerrera mentioned this pull request May 12, 2016

Improve /errlog JSON escaping logic and add tests #11208

Closed

nguerrera changed the title ~~[WIP] Bring /errorlog output up to SARIF draft 1.0.0-beta.4~~ [WIP] Bring /errorlog output up to SARIF draft 1.0.0-beta.5 May 12, 2016

nguerrera changed the title ~~[WIP] Bring /errorlog output up to SARIF draft 1.0.0-beta.5~~ Bring /errorlog output up to SARIF draft 1.0.0-beta.5 May 12, 2016

nguerrera added 6 commits May 13, 2016 14:04

Weaken JsonWriter's StreamWriter dependency to TextWriter

1851e12

Also, defer to TextWriter for NewLine character rather than hard-coding Environment.NewLine.

Add JSON escaping tests

10e1a9b

Remove unnecessary special-case for "'" in JsonWriter

450f6fe

Also remove outdated future improvement comment

Bring /errorlog output up to SARIF draft 1.0.0-beta.4

fe02e3f

Log rule metadata in /errorlog in official way

3826c36

nguerrera force-pushed the sarif-update branch from 5c3c3bc to f26c807 Compare May 13, 2016 21:10

Pilchie added the Area-Analyzers label May 15, 2016

mavasani mentioned this pull request May 16, 2016

Re-generate error log baselines for VSI tests #11332

Closed

Update /errorlog doc

a8dde7e

mavasani reviewed May 16, 2016
View reviewed changes

nguerrera mentioned this pull request May 16, 2016

Logical locations for diagnostics #11345

Open

agocke reviewed May 17, 2016
View reviewed changes

nguerrera merged commit b280c27 into dotnet:master May 17, 2016

nguerrera deleted the sarif-update branch May 17, 2016 17:23

mavasani reviewed May 17, 2016
View reviewed changes

		@@ -35,8 +37,8 @@ public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Versi
		_writer.WriteObjectStart(); // root
		_writer.Write("version", OutputFormatVersion);


		public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Version toolAssemblyVersion)
		public ErrorLogger(Stream stream, string toolName, string toolFileVersion, Version toolAssemblyVersion, CultureInfo culture)

Bring /errorlog output up to SARIF draft 1.0.0-beta.5 #11233

Bring /errorlog output up to SARIF draft 1.0.0-beta.5 #11233

Conversation

nguerrera commented May 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera May 12, 2016 • 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

nguerrera commented May 12, 2016

Choose a reason for hiding this comment

ghost May 12, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera May 12, 2016 • 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

ghost commented May 12, 2016

ghost commented May 12, 2016

mavasani commented May 12, 2016

mavasani commented May 12, 2016

nguerrera commented May 12, 2016

mavasani commented May 12, 2016

Choose a reason for hiding this comment

nguerrera commented May 12, 2016 • edited Loading

nguerrera commented May 13, 2016

jmarolf commented May 15, 2016

mavasani commented May 16, 2016

nguerrera commented May 16, 2016

mavasani commented May 16, 2016

mavasani commented May 16, 2016

mavasani commented May 16, 2016

nguerrera commented May 16, 2016

ghost commented May 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented May 16, 2016

mavasani commented May 17, 2016

nguerrera commented May 17, 2016

Choose a reason for hiding this comment

agocke commented May 17, 2016

ghost commented May 17, 2016

nguerrera commented May 17, 2016

mavasani commented May 17, 2016 • edited Loading

mavasani May 17, 2016 • edited Loading

Choose a reason for hiding this comment

nguerrera May 18, 2016 • edited Loading

Choose a reason for hiding this comment

mavasani commented May 17, 2016

nguerrera commented May 11, 2016 •

edited

Loading

nguerrera May 12, 2016 •

edited

Loading

ghost May 12, 2016 •

edited by ghost

Loading

nguerrera May 12, 2016 •

edited

Loading

nguerrera commented May 12, 2016 •

edited

Loading

mavasani commented May 17, 2016 •

edited

Loading

mavasani May 17, 2016 •

edited

Loading

nguerrera May 18, 2016 •

edited

Loading