Logical locations notes

[michaelcfanning]

Resolves:

#1183: In which we didn't set -1 properly as logical location index in case of absent logical location. This code also mishandled flowing the v1 location.decoratedName to v1 logicalLocation.decoratedName

#1182: The definitive fully qualified name for a v1 result refers to location.fullyQualifiedName first then, if absent, consults location.logicalLocationKey. The definitive logical location dictionary key reverses this process.

#1179: In which we failed to set logical location index properly for constructed stack frames (one of three v1 constructs, along with locations and annotated locations, that reference logical locations).

@lgolding

[ghost]

FullyQualifiedLogicalName [](start = 59, length = 25)

BUG: This isn't right. According to the spec (I looked as far back as the original v1 "working draft" from which the TC started its work), LogicalLocationKey is never guaranteed to be a valid FQLN. It is only guaranteed to match one of the property names in the run.logicalLocations dictionary.

If v1.FQLN is missing, your only choice is to omit v2.FQLN, so the correct code here is:

location = new Location
{
    FullyQualifiedLogicalName = v1Location.FullyQualifiedLogicalName, // Works whether or not it's present in the v1 file
    ...

It doesn't even help to use the key to find the logicalLocation object in run.logicalLocations dictionary. In v1, a logicalLocation object doesn't have a fullyQualifiedName property, so you'd have to walk the parent chain and glue the FQLN together yourself (and you'd have to know the language syntax to do it).

So there's no good answer. Just copy the FQLN from v1 to v2 and call it a day.
#Resolved

[ghost]

I think I might be contradicting what I commented in your previous PR. If so, my previous comment was wrong. You just can't get away with using a key as an FQLN.

---

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

[michaelcfanning]

Yes, this contradicts what we discussed previously but what you say here makes perfect sense.

---

In reply to: 244237488 [](ancestors = 244237488,244237410)

[ghost]

logicalLocationKey [](start = 23, length = 18)

Yes, good. #ByDesign

[ghost]

} [](start = 12, length = 1)

This is right but I wonder if this is clearer:

location.LogicalLocationIndex = -1;
if (logicalLocation != null && _v2LogicalLocationToIndexMap.TryGetValue(logicalLocation, out int logicalLocationIndexFromMap)))
{
    location.LogicalLocationIndex = logicalLocationIndexFromMap;
}
``` #WontFix

[ghost]

Why I think it's clearer:

1. Doesn't require an uninitialized variable.
2. Doesn't use a temporary, and so...
3. One line shorter.
4. if statement uses "positive logic".

YMMV.

---

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

[michaelcfanning]

I like where you're taking this. However, you neglect a key point, when TryGetValue fails, 'logicalLocationIndexFromMap' is initialized to the default of 0, which is not the actual default for logical location index (it is -1 as you know).

And so another subtlety in the format, our default values that do not correspond to C# primitive defaults require additional work in cases like this.

---

In reply to: 244239715 [](ancestors = 244239715,244239471)

[ghost]

Oh, right you are.

---

In reply to: 244372604 [](ancestors = 244372604,244239715,244239471)

[ghost]

; [](start = 106, length = 1)

Place lines 910-920 (this block of new code) within the test if (v1Run.LogicalLocations != null) { on Line 921. #Resolved

[michaelcfanning]

nice catch, thanks.

---

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

[ghost]

_ [](start = 32, length = 1)

Note to self: Since he's passing these two arguments, do they need to be instance fields? #Pending

[michaelcfanning]

This is a constantly confounding question. In general, I try to parameterize method calls with data that is relevant to its general operation. The use of instance data within a helper of this kind should represent data that 'slips' out of the core function. IOW, we are either utilizing some global initialization set previously (which is configurable) or we are opportunistically preserving some data that will be used later.

Realistically, it's hard to maintain a mental model of the code to know that these 'rules' are being followed. Also, this kind of coding in general complicates unit testability, requiring creation/initialization in some cases to test isolated helpers. A more predictably testable pattern is to author as many helpers as possible as static methods. The downside here, of course, is that if you do a good job factoring methods into small pieces, you may find a need to 'plumb' instance members through several layers in order to make it available to some low level helper.

Anyway, come in soon and have lunch (on me) and we'll discuss this specific topic thoroughly. It's been on my mind for precisely the reason you added this comment here.

---

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

[ghost]

logicalLocationKey [](start = 19, length = 18)

Yes yes yes. #ByDesign

[ghost]

fullyQualifiedNameToDecoratedNameMap [](start = 20, length = 36)

This variable name makes me worry whether the logic is correct.

In v1, you might have two logical locations with the same FQLN but different decorated names (if one was a type and one was a method). So I would expect to have a mapping from key to decorated name, not from FQLN to decorated name. This might become clear when I review the visitor. #Resolved

[ghost]

exists [](start = 77, length = 6)

BUG: This is the thing I worried about in SarifVersionOneToCurrentVisitor.cs. I would expect a mapping from key to decorated name, not from FQLN to decorated name, since there could be two logical locations with the same FQLN but different decorated names. In fact, come to think of it, one of the test files has this situation. #Resolved

[michaelcfanning]

Very nice catch. The PREfast samples have this condition. I believe all these samples, however, have been converted to pre-release v2. I will add a test for this case.

---

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

[ghost]

set [](start = 87, length = 3)

Setters not necessary for these two properties. #Resolved

[ghost]

( [](start = 54, length = 1)

StringComparison.Ordinal. #WontFix

[michaelcfanning]

This comparison (string.Equals) is one of the places in .NET where Ordinal is the default. This is true for other primitives such as the numerics. I don't make this explicit in code (for reasons of readability and maintainability) because I consider it a sort of essential fact about .NET that is helpful to know. (just as I suggest to people that they avoid unnecessary initialization of instance members). YMMV. :)

https://docs.microsoft.com/en-us/dotnet/csharp/how-to/compare-strings
"The most common methods that test for equality, String.Equals and String.Equality, use a case-sensitive ordinal comparison."

---

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

[ghost]

I was not aware of that.

---

In reply to: 244376530 [](ancestors = 244376530,244240657)

[ghost]

LogicalLocationKey [](start = 83, length = 18)

Just use the key as the index, for reason given above. #Resolved

[ghost]

Sorry, that comment is wrong. But so is the code. :-) You're trying to figure out what to use as the key, so the variable name should be key, and the code is:

string key = node.LogicalLocationKey ?? node.FullyQualifiedLogicalName;
KeyToDecoratedNameMap[key] = node.DecoratedName;

---

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

[ghost]

(This is the fix to the bug I was worried about.)

---

In reply to: 244373857 [](ancestors = 244373857,244240704)

[michaelcfanning]

Your statement isn't quite right, I mention the intent in the class comment. This code only works in cases where disambiguation is created, i.e., when a logical location key is used to resolve a collision. We don't need to persist all the other cases, because generally we have our hands on a fully qualified name already on those instances. Still open to making a change here if you see one.

---

In reply to: 244373857 [](ancestors = 244373857,244240704)

[ghost]

I still don't think this is right. You say that this code works "when a logical location key is used to resolve a collision." So consider the case:

location1 = {
    "fullyQualifiedLogicalName": "ambiguousFqln",
    "logicalLocationKey": "key1",
    "decoratedName": "dn1"
};

location2  = {
    "fullyQualifiedLogicalName": "ambiguousFqln",
    "logicalLocationKey": "key2",
    "decoratedName": "dn2"
};

fullyQualifiedLogicalName is the same, and non-null, for both locations. So when you visit location1, you will execute.

FullyQualifiedNameToDecoratedNameMap["ambiguousFqln"] = "dn1";

... and when you visit location2, you will execute

FullyQualifiedNameToDecoratedNameMap["ambiguousFqln"] = "dn2";

... losing dn1 entirely. With my suggested code, you will execute

KeyNameToDecoratedNameMap["key1"] = "dn1";
...
KeyNameToDecoratedNameMap["key2"] = "dn2";

... which is what you need.

---

In reply to: 244377445 [](ancestors = 244377445,244373857,244240704)

[michaelcfanning]

I'd already fixed this. I was confused by your statement in stand-up, thought we were discussing the other mapping scenario (to retrieve FQN in cases where there is a key collision)

---

In reply to: 244383433 [](ancestors = 244383433,244377445,244373857,244240704)

[ghost]

stack frame [](start = 18, length = 11)

annotated code location. #Resolved

[michaelcfanning]

cut-n-paste ftw

---

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

[ghost]

Crud. This test occurs three times, but you can't DRY it out to static bool NodeHasAmbiguousLogicalLocation because the node is of three different types. Still yet another validation of the design improvement where we moved these properties into the Location object. #ByDesign

[michaelcfanning]

Indeed. Almost left a comment for you on precisely this point (my inability to DRY due to the 'duck typing' of our previous design that inlined FQN and logical location keys. :)

---

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

[ghost]

🕐

[ghost]

Two substantive bugs and several nits. #Resolved

[ghost]

[ghost]

[michaelcfanning]

@lgolding thanks for your diligence cleaning this area up. I explicitly added tests that exhaustively test v1 key collisions for stack frames, annotated code locations, and locations. Everything now working.