Some LINQ that kills Unity every time

Going to report this one as a bug. Today I wrote a bit of LINQ that crashes Unity hard, the editor just shuts down.

Code (csharp):

1. return (sectorId == -1) ? 0 : faction[0].unit.Where(n => n.sectorId == sectorId && n.unitClass == UnitData.UnitClass.Diplomatic).Max(n => n.Leadership(FactionData.FactionType.Empire) as int?) ?? 0;

It's not actually as ugly as it looks at first glance... and the non-LINQ equivalent doesn't cause a crash:

Code (csharp):

1.         int max = 0;
2.         if(sectorId > -1)
3.         {
4.             foreach(UnitData u in faction[0].unit)
5.             {
6.                 if(u.sectorId == sectorId && u.unitClass == UnitData.UnitClass.Diplomatic)
7.                 {
8.                     int leadership = u.Leadership(FactionData.FactionType.Empire);
9.                     max = Mathf.Max(max, leadership);
10.                 }
11.             }
12.         }
13.         return max;

 

Not sure I agree with you there

I generally try to follow the rule of one statement per line. If you separate it into three lines(the Where, the Max and the ternary op), which line crashes Unity?

 

It's probably Unity's old-ass Mono implementation failing again. If you've got it downloaded, check if the beta for the new mono version crashes things.

I would guess that it's because you're doing something along the lines of:

Code (csharp):

1. return (bool b) ? (int i1) : (int? i2) ?? (int i3)

Which means that it's not super-unlikely that there's a bug somewhere in good old runtime that tries to stuff an int?-object into an int at.


After you've figured that out, make that Linq prettier:

Code (csharp):

1. //in UnitData
2. int GetDiplomacyFor(FactionData.FactionType faction) {
3.     if(unitClass != UnitClass.Diplomatic)
4.         return 0;
5.  
6.     return Leadership(faction) ?? 0;
7. }
8.  
9. //Query is now:
10. return(sectorId ==-1) ? 0 : faction[0].unit.Max(u => u.GetDiplomacyFor(FactionData.FactionType.Empire));

Or, you know, a helper in wherever the query is that takes both the unitData and the faction if that's more to your liking. The problem with how it stands is really that you're mixing ?: and ??, which means that the reader have to remember/look up the precedence of those two operators.

Also why is a list of UnitData named "unit"? Shouldn't it be "units"?

 

I don't find it especially hard to read, but I tend to write quite a bit of LINQ, and of course, I know what this is supposed to be doing, which helps a lot.

As for various things to make it "pretty"... I use LINQ to reduce line-count. If I wanted to spread it out, I'd probably just write the non-LINQ code and move on.

I've been on the fence about that style decision for many years. My preference is for how it reads when I'm accessing one element... unit[0] versus units[0].

Surprisingly, it is Max which causes the crash. First I tried this:

Code (csharp):

1.            int max = 0;
2.             var a = faction[0].unit.Where(n => n.sectorId == sectorId && n.unitClass == UnitData.UnitClass.Diplomatic);
3.             var b = a.Max(n => n.Leadership(FactionData.FactionType.Empire) as int?);
4.             max = b ?? 0;

Then I moved the cast to the last line but it still crashes on Max. (The Leadership function is just a simple switch that returns an int, it's used throughout the program and isn't a problem.)

Code (csharp):

1.            int max = 0;
2.             var a = faction[0].unit.Where(n => n.sectorId == sectorId && n.unitClass == UnitData.UnitClass.Diplomatic);
3.             var b = a.Max(n => n.Leadership(FactionData.FactionType.Empire));
4.             max = (b as int?) ?? 0;

That or it's something about iterating the Where predicate, when I break before the Max and inspect var a, I see it has set up for lazy evaluation, the unit list hasn't been processed yet. But there again, I use similar Where predicates throughout the program.

I know Max works, too, though, I use it elsewhere. So I'm not sure I'm much closer to identifying the specific problem.

 

You should check if it's actually the Where clause or if it's indeed the .Max. You'll have to force the Where to execute, though:

Code (csharp):

1.  
2. int max = 0;
3. var a = faction[0].unit.Where(n => n.sectorId == sectorId && n.unitClass == UnitData.UnitClass.Diplomatic);
4. a.ToList(); //actually do the iteration!
5. var b = a.Max(n => n.Leadership(FactionData.FactionType.Empire) as int?);
6. max = b ?? 0;

It's an interesting bug. Do you have a strip-down version that you could post? Would like to check it against the new runtime, and narrow down what's happening..


... by the way, why're you casting to 'int?' ? what's the return type of Leadership?

Reducing line-count shouldn't really be a goal. Increasing readability should. Linq is great there because it's saying what you want through terms like "from" and "where" instead of "foreach foreach foreach". Your original piece of code is really hard to read.

 

Hmm, it gets worse... I thought to force the Where eval by adding a .ToList line -- and now Unity crashes on the call to the function -- it never hits the breakpoint. Very strange. I can go to the line before the call to that function and break there, but trying to step into the function now crashes out.

As soon as I switch it back to the non-LINQ code breakpoints and stepping work again.

Comment out the non-LINQ and un-comment the LINQ -- any version of the LINQ code I've tried so far -- and now just the call to the function fails, it never even gets to the point that I was able to step through earlier. I rebooted, did a Clean Solution (not sure if that works with Unity projects, but it appears Attach to Unity does recompile), and it still happens.

Totally at a loss to explain that. I literally stepped through that code 20 minutes ago but can't reach it now.

At least the non-LINQ works.

 

Leadership() returns an int.

The "int? ?? 0" business prevents an exception when the Where clause returns an empty list... and the fact that I had to correct that means the original LINQ did run at some point. (And I know it's tempting to think some property accessor is blowing up or something like that, but this is very core code in the app and hasn't changed in a month or more... it's exactly as simple as it appears.)

I suppose it's time for a reproduction project ... but I'm about to get on the road for a holiday vacation. Maybe sometime this weekend.

 

pretty sure LINQ max doesn't handle nullible types without an IComparable. infact it doesn't make sense as Max would never return null, it would simply error out on empty collections (before it could even cast to int?). as you can see in your non-linq version your not using null-coalesing operators plus your using a default value.

for empty collections use DefaultIfEmpty(0).

Code (csharp):

1.  
2. if(sectorId == -1) return 0;
3.  
4. return faction[0].unit
5.         .Where(n => n.sectorId == sectorId && n.unitClass == UnitData.UnitClass.Diplomatic)
6.         .Select(n => n.Leadership(FactionData.FactionType.Empire))
7.         .DefaultIfEmpty(0)
8.         .Max();
9.  

 

This does work, and what you're saying makes sense, so thank you for that.

But just to clarify, it isn't that Max would return null, it's the default behavior of the "as" keyword -- if it can't make the cast (which would be the case when Max throws an exception), the "as" defaults to a null return. It's probably hacky, but it should work.

In the bigger picture, even if I'm Doing It Wrong (or perhaps just poorly...) the Unity editor should not respond by completely crashing. Nor does that explain the completely weird breakpoint problems I was having. (I did submit a bug report so I'll point them here for the repo.)

Since there is a crash to reproduce, and since my wife is not on the same vacation schedule I am this morning , I had time to build a very simple project that reproduces the crash. Put "Run Test" on the Main Camera.

 

The test does not crash on this version of Unity, so it's definitely a Mono bug! I tested with both .NET 2.0 and 4.6, and both give the correct results.

Since about half a year ago, Unity's responded to all bugs of this kind with "postponed until the Mono upgrade", so I wouldn't send a bug report. It'll probably be fixed come the upgrade planned for 5.5, though I'm not sure how much of the upgrade available in the beta will be in 5.5.

 

it wouldn't because you made the cast inside the Max() and since int is never null, casting to int? will also never be null. Max will error out on empty collections before it even touches your internal code.

take a look at this for loop example

Code (CSharp):

1. int max =-1;
2. int [] array = new array[]{};
3. for(int i=0;i<array.Length;i++)
4. {
5.     if(array.Length == 0) return 0;
6.  
7.     if(array[i]> max) max = array[i];
8. }
9. return max;
10.  

array is empty, thus the for loop never runs and the internal check is never even evaluated. your mistake is similar to assuming that you can put a check inside the for loop to see if its empty. Its the same thing with your max action. there is never going to be a case that code runs when the collection is empty. it'll instead throw an exception. The only way Max will return null in your code is if the collection wasn't empty, Leadership could return a nullable type, and already had a nullable value. However I assume it returns an int and thus it'll never return a null.

on that same subject since you're casting to int? in Max() then the entire Linq expression will return int?, thus your function will return a int (if sectorId is -1), or an int?. unless your function returns a dynamic type (which I discourage using) it only makes sense that it would break. I would check to see if you have any warnings or errors about implicit casting an int? to int. If the compiler is failing to catch that then I assume its as Baste says, its a Mono issue.

 

Every once in a while I think I should learn LINQ. Then I see threads like this.

 

It's pretty easy if you already know SQL or something similar. Or, you could get ReSharper to convert your loops for you until you learn the general format

 

I use a generator to write my SQL.

 

Me too It's called Linq (well, technically it's called Linq2Sql which is part of the Entity Framework, but you get my drift)

But yeah. learn Linq, it's awesome