-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SARIF OM changes for nullable int batch 1 #2650
SARIF OM changes for nullable int batch 1 #2650
Conversation
@@ -106,7 +106,7 @@ public static Action<WriteContext> GetWriter(string columnName) | |||
writers["Kind"] = (c) => { c.Writer.Write(c.Result.Kind.ToString()); }; | |||
writers["Level"] = (c) => { c.Writer.Write(c.Result.Level.ToString()); }; | |||
writers["Message.Text"] = (c) => { c.Writer.Write(c.Result.Message?.Text ?? ""); }; | |||
writers["OccurrenceCount"] = (c) => { c.Writer.Write(c.Result.OccurrenceCount); }; | |||
writers["OccurrenceCount"] = (c) => { c.Writer.Write(c.Result.OccurrenceCount?.ToString() ?? ""); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1740,6 +1744,7 @@ | |||
"charLength": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1709,24 +1709,28 @@ | |||
"startLine": { | |||
"description": "The line number of the first character in the region.", | |||
"type": "integer", | |||
"default": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what's odd about this is we have a default of -1 for other things, why not this?
how can a default be lower than the minimum value? seems odd.
@@ -39,24 +39,32 @@ public virtual SarifNodeKind SarifNodeKind | |||
/// The line number of the first character in the region. | |||
/// </summary> | |||
[DataMember(Name = "startLine", IsRequired = false, EmitDefaultValue = false)] | |||
[DefaultValue(0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we're making the same mistake though! '0' is a default value that effectively means it isn't there, so we should have a nullable type here, again.
This is more painful from an API use standpoint, maybe too painful! But everything would be consistent.
I do see from the spec that the region class in particular has complex/unusual default value semantics...
@@ -221,6 +221,16 @@ | |||
} | |||
} | |||
], | |||
"ExternalProperties.Schema": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replied in the other comment.
@@ -1075,6 +1095,26 @@ | |||
} | |||
} | |||
], | |||
"TranslationMetadata.DownloadUri": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how we missed them.
I found them by test not eye.
why is this still here? :) when will it go away? In reply to: 1491078338 Refers to: src/Sarif/Autogenerated/Address.cs:43 in fefed9a. [](commit_id = fefed9a, deletion_comment = False) |
@@ -38,6 +38,7 @@ public virtual SarifNodeKind SarifNodeKind | |||
/// The URI of the JSON schema corresponding to the version of the external property file format. | |||
/// </summary> | |||
[DataMember(Name = "schema", IsRequired = false, EmitDefaultValue = false)] | |||
[JsonConverter(typeof(Microsoft.CodeAnalysis.Sarif.Readers.UriConverter))] | |||
public virtual Uri Schema { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a 1x1 mapping between this type, URI and the JsonConverter attribute. That could have been the way we handled this in the codegen hints + jschema (i.e., provide a way to emit a specific attribute for every instance of a property with a specific return type). Just an idea, can you confirm this feature does not already exist? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UriConverter class is from SDK not JSchama so we can not directly always generate this attribute when Uri type in JSchema.
We could have support hint that base on type, not just name, then we can have all this declared once. I checked the hint is only based on name.
An other way is we could have still use base on name but support partial wild card e.g. *.*.Uri*
then we can have it declared declared once, except this kind of special one Schema
that does no have Uri in the name.
But I checked we only support * at the first part only: *.Uri
.
private static string MakeHintDictionaryKey(string typeName, string propertyName)
{
return typeName + "." + propertyName.ToPascalCase();
}
Let me know which direction, I can create a work item and work on it.
@@ -39,24 +39,32 @@ public virtual SarifNodeKind SarifNodeKind | |||
/// The line number of the first character in the region. | |||
/// </summary> | |||
[DataMember(Name = "startLine", IsRequired = false, EmitDefaultValue = false)] | |||
[DefaultValue(0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -116,8 +128,14 @@ public virtual SarifNodeKind SarifNodeKind | |||
/// </summary> | |||
public Region() | |||
{ | |||
StartLine = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, there are 2 thing here:
- the line column numbers will be gone after we work on it and regenerates, for this PR we can ignore all changes related to them for now,
- CharLength and ByteLength is a real change here. It is autogenerated.
I have created an issue in JSchema:
Improvement: remove unnecessary initialization of property default value in ToDotNet jschema#172
@@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.Sarif | |||
/// A physical or virtual address, or a range of addresses, in an 'addressable region' (memory or a binary file). | |||
/// </summary> | |||
[DataContract] | |||
[GeneratedCode("Microsoft.Json.Schema.ToDotNet", "2.1.0.0")] | |||
[GeneratedCode("Microsoft.Json.Schema.ToDotNet", "2.3.0.0")] | |||
public partial class Address : PropertyBagHolder, ISarifNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did have compared that every file in the notyetautogenerated folder have difference between the raw auto generated one, when work on these I believe some tests are needed, I think for the separate change.
@@ -29,7 +29,7 @@ public static IExpressionEvaluator<Result> ResultEvaluator(TermExpression term) | |||
case "message.text": | |||
return new StringEvaluator<Result>(r => r.Message?.Text, term, StringComparison.OrdinalIgnoreCase); | |||
case "occurrencecount": | |||
return new LongEvaluator<Result>(r => r.OccurrenceCount, term); | |||
return new LongEvaluator<Result>(r => r.OccurrenceCount ?? 1, term); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if we should include this in a spec errata ---- I agree with this and will need to talk to you if we consider "the spec is missing this default as 1 by mistake". If so we can keep the OccurrenceCount as not nullable and have default as 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All types with a default must be nullable!! We can discuss in stand-up today. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All types with a default must be nullable!! ---- @michaelcfanning I have question about this because it is the opposite of what my understanding, my understanding and also my PR that lead to this was that was reviewed and checked in.
I created an email to describe details.
Also please review that change that was checked in: microsoft/jschema#167
And because integers without default we HAVE to make them nullable, if with a default you also want them to be nullable that would be basically everything should be nullable.
observe that the minimum value here matches the default value! In reply to: 1491093798 In reply to: 1491093798 Refers to: src/Sarif/Schemata/sarif-2.1.0-rtm.6.json:1740 in fefed9a. [](commit_id = fefed9a, deletion_comment = False) |
@@ -1709,24 +1709,28 @@ | |||
"startLine": { | |||
"description": "The line number of the first character in the region.", | |||
"type": "integer", | |||
"default": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -225,6 +225,11 @@ private void ProcessLine(string logFileLine, ref int nestingLevel, Result result | |||
} | |||
} | |||
|
|||
if (threadFlowLocation.NestingLevel == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct, this is code for converting StaticDriverVerifier result to SARIF, the NestingLevel here is converted so if it is 0 then it means 0, not unknown. I have reverted this change, and fixed the test expected outputs to have this property.
@@ -43,7 +43,7 @@ | |||
}, | |||
{ | |||
"location": { | |||
"uri": "file:/c:/src/file.c", | |||
"uri": "file:///c:/src/file.c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -680,7 +680,7 @@ public void SarifLogger_LogsStartAndEndTimesByDefault() | |||
// Other properties should be empty. | |||
invocation.CommandLine.Should().BeNull(); | |||
invocation.WorkingDirectory.Should().BeNull(); | |||
invocation.ProcessId.Should().Be(0); | |||
invocation.ProcessId.Should().Be(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, batch fixed all existing ones.
@@ -1709,24 +1709,28 @@ | |||
"startLine": { | |||
"description": "The line number of the first character in the region.", | |||
"type": "integer", | |||
"default": 0, | |||
"minimum": 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have successfully added a new CodeQL configuration |
CharOffset = -1; | ||
CharLength = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
ByteOffset = -1; | ||
ByteLength = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
@@ -116,8 +128,14 @@ | |||
/// </summary> | |||
public Region() | |||
{ | |||
StartLine = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
@@ -116,8 +128,14 @@ | |||
/// </summary> | |||
public Region() | |||
{ | |||
StartLine = 0; | |||
StartColumn = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
@@ -116,8 +128,14 @@ | |||
/// </summary> | |||
public Region() | |||
{ | |||
StartLine = 0; | |||
StartColumn = 0; | |||
EndLine = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
StartLine = 0; | ||
StartColumn = 0; | ||
EndLine = 0; | ||
EndColumn = 0; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor
Thanks, I see your comments about the changes about the 4 line/column numbers. |
Will need to fix this in the future changes, this change set is only those triggered by the new version of JSchema, must act or else will not build. In reply to: 1491078338 Refers to: src/Sarif/Autogenerated/Address.cs:43 in fefed9a. [](commit_id = fefed9a, deletion_comment = False) |
Microsoft.Json.Schema.*
packages from 2.1.0.0 to 2.3.0.0. #2650Artifact.Offset
,Invocation.ExitCode
,Invocation.ExitSignalNumber
,Invocation.ProcessId
,Result.OccurrenceCount
,EdgeTraversal.StepOverEdgeCount
,Notification.ThreadId
,StackFrame.ThreadId
,ThreadFlowLocation.NestingLevel
, andWebResponse.StatusCode
properties fromint
toint?
type. #2650The reason of this change is, the upgrade of the
Microsoft.Json.Schema.*
packages exposed an issue that a few int properties should be nullable but it is current not nullable in our C# code.Due to impact of the 4 line/columns numbers for
Region
, proposed for a separated PR and will need a confirmation for the details before start.