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

IOperation Test Porting Pt 3 #51206

Merged
merged 14 commits into from
Feb 25, 2021
Merged

IOperation Test Porting Pt 3 #51206

merged 14 commits into from
Feb 25, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented Feb 13, 2021

This ports the last of the commits from #49534 and addresses feedback on them. The only remaining test hook failures in master were introduced by the UsedAssemblyReferences branch and Aleksey is taking a look at those. If that isn't fixed by the time this is merged, I'll turn those tests off for the test hook when I enable the test hook in CI. After investigation, these failures are invalid, and the assert is removed by the last commit in this PR.

As in my previous porting PRs, ported commits are prefixed with Port:, and fixup commits are prefixed with Fixup:. There are 4 new, previously unreviewed commits at the end that are not prefixed. See the commit descriptions for what each one does.

… if they do not have one.

(cherry picked from commit 4af2613)
@333fred 333fred marked this pull request as ready for review February 16, 2021 20:10
@333fred 333fred requested a review from a team as a code owner February 16, 2021 20:10
@333fred
Copy link
Member Author

333fred commented Feb 16, 2021

@dotnet/roslyn-compiler @AlekseyTs for review.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 17, 2021

Done with review pass (commit 9) #Closed

* Simplify named argument binding by making all named args specifically BoundAssignmentOperators. They already were, this carries the knowledge through the rest of the code.
* Add tests for invalid attribute binding scenarios.
* Only bind constructor params for error recovery in bad cases.
* Use the correct binder when binding for error recover to avoid runaway binding.
@333fred
Copy link
Member Author

333fred commented Feb 19, 2021

@AlekseyTs addressed feedback. I've split the resolutions into separate commits as they followup on different iteration comments.


In reply to: 780630534 [](ancestors = 780630534)

@333fred
Copy link
Member Author

333fred commented Feb 22, 2021

@AlekseyTs @dotnet/roslyn-compiler for review.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 23, 2021

Done with review pass (commit 11) #Closed

@333fred
Copy link
Member Author

333fred commented Feb 25, 2021

@AlekseyTs addressed feedback.


In reply to: 784305142 [](ancestors = 784305142)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 13), assuming CI is passing.

@333fred
Copy link
Member Author

333fred commented Feb 25, 2021

@dotnet/roslyn-compiler for a second review please.

@@ -942,8 +942,6 @@ private TypeSymbol GetResolvedType()
{
if ((object)_resolved == null)
{
Debug.Assert(_underlying.IsSafeToResolve());
Copy link
Member

@jcouv jcouv Feb 25, 2021

Choose a reason for hiding this comment

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

Why remove this assertion? #Closed

@jcouv
Copy link
Member

jcouv commented Feb 25, 2021

    /// The result of this method captures some AnalyzedArguments, which must be free'ed by the caller.

Please update the comment (the result now also holds pooled builders which the caller is responsible for freeing) #Closed


Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs:305 in 76e0ccd. [](commit_id = 76e0ccd, deletion_comment = False)

else
{
boundConstructorArguments = analyzedArguments.ConstructorArguments.Arguments.SelectAsArray(
(arg, attributeArgumentBinder) => attributeArgumentBinder.BindToTypeForErrorRecovery(arg),
Copy link
Member

@jcouv jcouv Feb 25, 2021

Choose a reason for hiding this comment

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

(arg, attributeArgumentBinder) [](start = 20, length = 30)

static? #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 13)

@jcouv jcouv self-assigned this Feb 25, 2021
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 14)

@333fred 333fred merged commit 11495c1 into dotnet:master Feb 25, 2021
@333fred 333fred deleted the iop-part3 branch February 25, 2021 23:17
@ghost ghost added this to the Next milestone Feb 25, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants