Skip to content

Checking for writeable indexers in Use Range Operator feature#44951

Merged
jmarolf merged 23 commits intodotnet:masterfrom
m-redding:bug-43202
Aug 10, 2020
Merged

Checking for writeable indexers in Use Range Operator feature#44951
jmarolf merged 23 commits intodotnet:masterfrom
m-redding:bug-43202

Conversation

@m-redding
Copy link
Copy Markdown
Contributor

@m-redding m-redding commented Jun 8, 2020

fixes #43202

CC: @JavierMatosD

@sharwell Could you take a look at the changes made to test case "TestNonStringType_Assignment"? The original version seemed to contradict the bug fix we made, but we want to be sure that the test case is not still valid under the new expected behavior.

@dnfadmin
Copy link
Copy Markdown

dnfadmin commented Jun 8, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@jmarolf
Copy link
Copy Markdown
Contributor

jmarolf commented Jun 8, 2020 •
edited by sharwell
Loading 

    [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseRangeOperator)]

I would add [WorkItem(43202, "https://github.com/dotnet/roslyn/issues/43202")] at the top so that we can find why this change was made

Refers to: src/Analyzers/CSharp/Tests/UseIndexOrRangeOperator/UseRangeOperatorTests.cs:323 in 5351a55. [](commit_id = 5351a55, deletion_comment = False)

CyrusNajmabadi
CyrusNajmabadi previously requested changes Jun 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core issue was the case of:

struct S
{
  public ref S Slice(int start, int length) => throw null;
  public int Length { get; }
  public S this[System.Range r] { get => default; }
}

here Slice returns by-ref. i.e. ref S Slice.... However, the indexer does not.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 9, 2020

Thanks for the contribution!

m-redding and others added 4 commits June 16, 2020 16:59
@m-redding @CyrusNajmabadi
Update src/Analyzers/CSharp/Analyzers/UseIndexOrRangeOperator/CSharpU…
07c2d51 
…seRangeOperatorDiagnosticAnalyzer.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
@m-redding @CyrusNajmabadi
Update src/Analyzers/CSharp/Analyzers/UseIndexOrRangeOperator/CSharpU…
217d591 
…seRangeOperatorDiagnosticAnalyzer.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
resolving issues from code review
563c9c3
typo fix
ed9d641
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 17, 2020

Can we fixul the titel of this PR to be clearer? Thanks!

@m-redding m-redding changed the title Bug 43202 Checking for writeable indexers in Use Range Operator feature Jun 18, 2020
Updates to feedback
b08833b 
I attempted to move the if-statement to the ComputeMemberInfo method,
however it seems that the necessary information for the
IsLeftSideOfAnyAssignExpression() can only been found in the Analyzer,
so I decided to keep the if-statement where it was previously.
@m-redding
Copy link
Copy Markdown
Contributor Author

m-redding commented Jul 23, 2020

Copying here to make this clearer: As stated in commit (b08833b) I attempted to move the if-statement to the ComputeMemberInfo method, however it seems that the necessary information for the IsLeftSideOfAnyAssignExpression() can only been found in the Analyzer, so I decided to keep the if-statement where it was previously.

@m-redding m-redding requested a review from jmarolf July 27, 2020 17:08
@m-redding m-redding dismissed CyrusNajmabadi’s stale review August 4, 2020 17:15

I addressed the feedback in the comment left above that was requesting changes.

jmarolf
jmarolf approved these changes Aug 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

CyrusNajmabadi
CyrusNajmabadi reviewed Aug 4, 2020
View reviewed changes
...alyzers/CSharp/Analyzers/UseIndexOrRangeOperator/CSharpUseRangeOperatorDiagnosticAnalyzer.cs Outdated
// Second arg needs to be a subtraction for: `end - e2`. Once we've seen that we have
// that, try to see if we're calling into some sort of Slice method with a matching
// indexer or overload
var valid = infoCache.TryGetMemberInfo(targetMethod, out var memberInfo);
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this extracted?

Copy link
Copy Markdown
Contributor Author

@m-redding m-redding Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed, I had been using valid for something else later but then removed it

@jmarolf jmarolf merged commit aa4ecc0 into dotnet:master Aug 10, 2020
@ghost ghost added this to the Next milestone Aug 10, 2020
@RikkiGibson RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jmarolf jmarolf jmarolf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.8.P2

Development

Successfully merging this pull request may close these issues.

Use Range Operator should check for writable indexer

7 participants

@m-redding @dnfadmin @jmarolf @CyrusNajmabadi @sharwell @RikkiGibson @jinujoseph