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.

Sign up for GitHub

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

Jump to bottom

ExpressionLambdaRewriter.VisitArrayAccess check seems off #6805

Closed
bartdesmet opened this issue Nov 16, 2015 · 3 comments · Fixed by #7554
Closed

ExpressionLambdaRewriter.VisitArrayAccess check seems off #6805

bartdesmet opened this issue Nov 16, 2015 · 3 comments · Fixed by #7554
Assignees
@VSadov
Labels
Area-Compilers Bug
Milestone
1.2

Comments

@bartdesmet
Copy link 

if (node.Type != _int32Type)

looks like it should be

if (arg.Type != _int32Type)
@bartdesmet
Copy link
Author

Seems it always works now because node.Type can never be an Int32 given that it's an array type. We just may have a spurious conversion. It'd be interesting to see whether the old compiler had this too; if we take away the Convert node, some runtime expression analyzers may fail because they relied on it being there... :-(

@jaredpar jaredpar added the Bug label Dec 2, 2015
@jaredpar jaredpar added this to the 1.2 milestone Dec 2, 2015
@jaredpar
Copy link
Member

jaredpar commented Dec 2, 2015

That definitely looks like a bug. @VSadov can you take a look at this.

@VSadov
Copy link
Member

VSadov commented Dec 17, 2015

Looks like a bug.
It does not result in any differences since array type is never an int, but then ConvertIndex checks for index being int32 anyways.

VSadov added a commit to VSadov/roslyn that referenced this issue Dec 17, 2015
@VSadov
Trivial fix for 6805.
7b81dc5 
Just fixing the easy-out check that verifies if index conversion is needed at all. Currently the condition is always true - likely a typo.
Otherwise, the part that creates conversion does the right thing regardless, so the end result should be the same.

Fixes dotnet#6805
@VSadov VSadov mentioned this issue Dec 17, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@VSadov VSadov

Labels
Area-Compilers Bug
Projects
None yet
Milestone
1.2
Development

Successfully merging a pull request may close this issue.

Trivial fix for 6805. VSadov/roslyn
4 participants
@jaredpar @gafter @bartdesmet @VSadov