Skip to content
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

LINQ immediate query execution not caught for type checking #1244

Closed
itmitica opened this issue Feb 16, 2017 · 22 comments
Closed

LINQ immediate query execution not caught for type checking #1244

itmitica opened this issue Feb 16, 2017 · 22 comments

Comments

@itmitica
Copy link

Environment data

dotnet --info output:
.NET Command Line Tools (1.0.0-preview2-1-003177)

Product Information:
Version: 1.0.0-preview2-1-003177
Commit SHA-1 hash: a2df9c2576

Runtime Environment:
OS Name: ubuntu
OS Version: 16.04
OS Platform: Linux
RID: ubuntu.16.04-x64

VS Code version:
Version 1.9.1
Commit f9d0c687ff2ea7aabd85fb9a43129117c0ecf519
Date 2017-02-08T23:44:55.542Z
Shell 1.4.6
Renderer 53.0.2785.143
Node 6.5.0

C# Extension version:
1.7.0

Steps to reproduce

Make a LINQ interrogation over a array, using an immediate query execution operator: ToArray

        public static void Main(string[] args)
        {
            int[] fib = new [] {0,1,1,2,3,5};
            IEnumerable<int> fib2 = fib.Where(x => x > 2).ToArray();
            Console.WriteLine($"{fib2[0]");
        }

Expected behavior

Catch type changing from collection to array here: IEnumerable<int> fib2 = fib.Where(x => x > 2).ToArray();.

Actual behavior

Catches type change here: Console.WriteLine($"{fib2[0]");.

@duraz0rz
Copy link

duraz0rz commented Feb 16, 2017

Array implements IEnumerable<T>, so that particular line with ToArray is valid. dotnet build has the same build error on the same line: "Cannot apply indexing with [] to an expression of type 'IEnumerable<int>"

@DustinCampbell
Copy link
Member

This is an expected error. IEnumerable<int> does not provide an indexer. Change that to var and it'll work fine.

@itmitica
Copy link
Author

itmitica commented Feb 16, 2017

@DustinCampbell
Hmmm... No! I'm not debugging. :) I could have used int[] instead and have properly used the correct type qualification.

I think that having IEnumerable<int> at one end of an assignment and ToArray() at the other end of it, in a strongly typed language, that should be flagged.

@duraz0rz
Yes, dotnet build flags it correctly, that was my point actually. There's no squiggly red line on that line, but on the later line, like ToArray() is not an immediate operator.

toarray

@DustinCampbell
Copy link
Member

I think that having IEnumerable at one end of an assignment and ToArray() at the other end in a strongly typed language should be flagged.

Why would this be flagged? IEnumerable<int> is an interface that is implemented by int[], so that statement is completely legal. The error occurs when you try to index into the variable fib2, which has a type of IEnumerable<int>

@itmitica
Copy link
Author

OK.

@DustinCampbell
Copy link
Member

To be clear, this is how the C# language works. I get exactly the same error with dotnet build as I do with VS Code in your example. If you believe the language should work differently, you're welcome to file an issue at https://github.com/dotnet/csharplang.

@itmitica
Copy link
Author

No, I also think C# works correctly. And it's not the same error. VS Code has no error hinted at line 13, while the compiler points it there. VS Code hints an error at line 14, I'm talking about the squiggly red line.

@DustinCampbell
Copy link
Member

Well, there isn't error on line 13 and the compiler doesn't point there. Here's what I get with dotnet build:

Program.cs(14,35): error CS0021: Cannot apply indexing with [] to an expression of type 'IEnumerable<int>' [C:\Projects\test2\test2.csproj

@itmitica
Copy link
Author

itmitica commented Feb 16, 2017

Well, there is: ToArray() doesn't actually results in an array on line 13. ToArray() is not a deferred LINQ operator.

EDIT: And yes, you are right.

@DustinCampbell
Copy link
Member

Well, there is: ToArray() doesn't actually results in an array on line 13. ToArray() it's not a differed LINQ operator.

Sorry, but that didn't make sense to me. The expression fib.Where(x => x > 2).ToArray() has a type of int[]. This is assignable to fib2 because int[] is implicitly convertible to IEnumerable<int>. So, line 13 should not be an error. This is semantically legal C#.

You mentioned that you see a different error from the C# compiler than you do in VS Code, correct? Could you provide the error that you're getting from the compiler?

@itmitica
Copy link
Author

itmitica commented Feb 16, 2017

No, I was wrong about that, the compiler and VS Code both point to the same spot. You were correct in closing the issue.

The problem I see is IEnumerable<int> and ToArray() on opposites sides of an assignment working as a non-deferred operation only until being used later on, with specific array operators.

@DustinCampbell
Copy link
Member

The problem I see is IEnumerable and ToArray() on opposites sides of an assignment working as a non-deferred operation only until being used using specific array operators.

I see where the confusion is now -- thanks! Unfortunately, ToArray is not a deferred operation like Where or Select. Because the result of ToArray is an array, that array must be allocated an populated as a result, which means that the IEnumerable<T> that it's operating on must be fully iterated over. In essence, it has the result of running the query. This is very similar to calling Sum() which must iterate the full IEnumerable<int> to produce a result. Simply, converting the result of ToArray to an IEnumerable<int> does not make it deferred again.

@itmitica
Copy link
Author

I think I narrowed it down: IEnumerable<T> acts as a nullify-er for ToArray(). The fix for line 14 is to simply apply ToArray again. It doesn't seem right to me.
toarraydouble

@DustinCampbell
Copy link
Member

I'm not sure what you might be nullify-er.

There are a couple of fixes I could imagine. Here's a couple:

  1. Use var to infer the type of fib2 as an int[]. After all, why does it need to be of type IEnumerable<int>?
var fib2 = fib.Where(x => x > 2).ToArray();
Console.WriteLine($"{ fib2[0] }");
  1. Don't call ToArray() at all.
var fib2 = fib.Where(x => x > 2);
Console.WriteLine($"{ fib2.First() }");

@itmitica
Copy link
Author

The fact that is avoidable or fixable is not relevant, I'm not trying to debug my real code.

I just spotted an anomaly: calling ToArray() on line 13 doesn't really have an effect, I have to use ToArray() again if I want an array out of the query. Seems odd, to say the least.

toarrayisignored

@DustinCampbell
Copy link
Member

It does have an effect. The problem is that you immediately converted it to an IEnumerable<int>, which is completely unnecessary.

@itmitica
Copy link
Author

I was under the impression that final ToArray() is the one determining the final type, not the type annotation. At least, that is what it seems logic to me.

@DustinCampbell
Copy link
Member

DustinCampbell commented Feb 16, 2017

ToArray() determines the final type of that expression. That is, the expression fib.Where(x => x > 2).ToArray() has a type of int[]. However, that expression appears on the right-hand side of a simple assignment expression where the left-side is a variable of type IEnumerable<int>. So, the result of ToArray() is implicitly converted to the type of fib2.

Please refer to the C# specification: https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#simple-assignment.

The important bit is here:

The run-time processing of a simple assignment of the form x = y consists of the following steps:

  • If x is classified as a variable:
    • x is evaluated to produce the variable.
    • y is evaluated and, if required, converted to the type of x through an implicit conversion.

It's that last bullet that's critical here.

  • y is evaluated -> fib.Where(x => x > 2).ToArray() is evaluated and produces a type of int[].
  • converted to the type of x through an implicit conversion -> The int[] is implicitly converted to the type of fib2, which you've specified in your code as IEnumerable<int>.

Essentially, this is doing exactly what your code is telling it to do.

@itmitica
Copy link
Author

itmitica commented Feb 16, 2017

I opened an issue on csharplang as you suggested. My opinion is that the behavior of implicit conversion through assignment is not optimal in this case. A pretty big gotcha for me. I was expecting an error like "you can't assign yada yada", if array operations are not possible on the query result, no matter how legal the conversion.

@DustinCampbell
Copy link
Member

Just to set expectations, I wouldn't expect this to go very far. This is how the language (and other languages like C and Java) have worked for years. Changing this behavior would be a major breaking change.

@itmitica
Copy link
Author

I can live with that. It's been a learning experience. Thank you 👍

@DustinCampbell
Copy link
Member

No problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants