Possible unintended reference comparison worked as intended

Question

I have code similar to the following:

this.Session[key] = "foo";

if ((this.Session[key] ?? string.Empty) == "foo")
{
    //do stuff
}

This, of course, creates a "Possible unintended reference comparison worked as intended" situation. The solution to this is well documented here, and I already knew that the fix was to cast the session variable to a string as soon as I saw the code.

However, the code is years old and has never been changed since it was originally written. Up until this week in our test environment, the if statement evaluated as true and executed the //do stuff section. This erroneous code IS STILL WORKING as intended in our production environment.

How can this be? There's no reason this code as written should have ever worked as intended; and yet it did, and still does in production. And what would have changed to make this code that should not have worked but did, suddenly stop working (or rather, behave like it always should have)?

I'm confused by two things in this question. First, the question contains a logical fallacy: the action of the code has always been the same. It compares two references for equality. If it produces true, that's because the references are equal. If it produces true consistently, then the references are consistently equal. I don't understand what you're confused about here. Second, I cannot for the life of me figure out what the null coalescing operator is intended to mean here. The code means "if the value is null, compare empty to null", but why? the value is already not "foo" if null! — Eric Lippert, Dec 15 '15 at 21:32
How exactly is the `Session[key]` set? What is it compared to? What version of .NET framework do you use in test and production environments? There must be some difference between the two environments that caused string interning to work differently. The code you posted uses 2 string literals - afaik these will always be interned unless interning is turned off at assembly level. — Jakub Lortz, Dec 15 '15 at 22:28
@EricLippert - I've overly simplified my example from the actual implementation. In the real code, the session variable is being set in another module from a query string parameter that may or may not exist. The null coalesce is there because if the session variable ends up being null, it will throw an exception. Also, my confusion is over the fact that the code used to consistently evaluate true. Now it is consistently evaluating false. — jtrohde, Dec 16 '15 at 00:27
If the session variable is null, the null coalesce will not help to prevent an exception, it only converts a null value retrieved from the session variable. but for this to occur the session variable must be non-null, otherwise indexing will fail. And he null coalesce operator will not help in the comparison because `string.Empty=="foo"` and `null=="fool"` both return `false` (no exception is thrown if you compare a string to `null`). — Olivier Jacot-Descombes, Dec 16 '15 at 14:29
@Olivier Jacot-Descombes: That's true. The code predates me, so I can't take credit for it, but I should have recognized the coalesce was redundant. — jtrohde, Dec 16 '15 at 16:01

score 4 · Accepted Answer · edited Jun 20 '20 at 09:12

4

The string literal "foo" is interned. It means that every time it's used, the same object is referenced.

The common language runtime conserves string storage by maintaining a table, called the intern pool, that contains a single reference to each unique literal string declared or created programmatically in your program. Consequently, an instance of a literal string with a particular value only exists once in the system.

For example, if you assign the same literal string to several variables, the runtime retrieves the same reference to the literal string from the intern pool and assigns it to each variable.

That's why object.ReferenceEquals("foo", "foo") is true.

If you do the same with a dynamically created string, it will not be interned and the references will not be equal

object str = new StringBuilder().Append("f").Append("oo").ToString(); // str = "foo"
object.ReferenceEquals(str, "foo");                                   // false

String interning can work differently depending on implementation, that's why you can get different behavior on different environments when comparing strings by reference.

edited Jun 20 '20 at 09:12

Community

1
1

answered Dec 15 '15 at 21:07

Jakub Lortz

14,616
3
25
39

It would be good it you also included a example of how to do it "correctly" (`Object.Equals((this.Session[key] ?? string.Empty), "foo")`, using Object.Equals checks for overloads of the Equals function so works when both objects are not their true types and with nulls) – Scott Chamberlain Dec 15 '15 at 21:23
@ScottChamberlain That's a very roundabout way to correct the code. The most straightforward way would be to cast `Session[key]` to string and use an appropriate string comparison. – phoog Dec 15 '15 at 21:28
@phoog `Session[key]` may or may not hold a `string`. I agree that it most sane programs it will always be the the same type but it has already been shown the legacy code already makes mistakes so I don't know if it can be trusted that it will never store anything except a string in the session variable. – Scott Chamberlain Dec 15 '15 at 21:30
@ScottChamberlain fair enough. If I had to code for the possibility that that the return value might be something other than a string, though, I would probably handle that explicitly rather than trusting that nobody would convert the Object.Equals call back into `==` at some point in the future. – phoog Dec 15 '15 at 21:33
1

@ScottChamberlain The OP knows how to fix the code to use the string overload of `==`. I think putting it in the answer will only add noise. – Jakub Lortz Dec 15 '15 at 21:35
@ScottChamberlain `Session[key]` could also contain an instance of `public class EqualToEverything { public override bool Equals(object obj) => true; }`. I'd prefer an InvalidCastException. – Jakub Lortz Dec 15 '15 at 21:36
Yoda-compare FTW: "foo".Equals(Session[key]) – Mark Brackett Dec 15 '15 at 22:13
Mystery solved, with help from this clue! Previously the session variable and the variable it was being compared to were subject to interning and the reference comparison evaluated as true. In our test region, however, we've implemented our own custom session database. The values stored here are not subject to interning, which is why the reference comparison now fails. – jtrohde Dec 16 '15 at 01:11
Interning occurs only for literal strings (e.g. string constants) and when String.Intern is called explicitly. `Object.ReferenceEquals("foo","foo".ToLower())` yields **false**! See 1st code example in the "Remarks" section, here: [String.Intern Method (String)](https://msdn.microsoft.com/en-us/library/system.string.intern%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396) – Olivier Jacot-Descombes Dec 16 '15 at 14:19
@OlivierJacot-Descombes What you say is true in MS implementation of CLR, but I think it's an implementation detail. I can't find any information that interning at run-time is prohibited. For example empty string is interned at run-time by some versions of .NET ([source](https://blogs.msdn.microsoft.com/ericlippert/2009/09/28/string-interning-and-string-empty/)) – Jakub Lortz Dec 16 '15 at 18:24
Yes, at runtime you can intern a string by calling `String.Intern`. Therefore, in my answer I said "You were lucky that the comparison worked!". You just cannot rely on interning and **must** ensure that a true string comparison is performed and not a reference comparison. – Olivier Jacot-Descombes Dec 16 '15 at 20:25

score 2 · Answer 2 · edited May 23 '17 at 10:28

You were lucky that the comparison worked! In your example the string "foo" is interned, i.e. both string literals are stored once and have both the same reference. However, if one of the two foo's is defined in another assembly or is de-serialized or is somehow constructed in code (e.g. string s = "f"; string t = s + "oo";), then the references might be different. Since Session[key] is typed as object a reference comparison will be performed.

The coalesce is not necessary, however a casting is:

if ((string)Session[key] == "foo") { ... } // This will perform a textual comparison.

You could also write this (since Equals is polymorphic):

if (Session[key].Equals("foo")) { ... } // This will perform a textual comparison.

It is not enough to compare 2 string values, they must statically be typed as string in order to be compared as strings.

Related, extremely interesting post of Jon Skeet on this subject: https://stackoverflow.com/a/3678810/880990

Possible unintended reference comparison worked as intended

2 Answers2