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

Asserts in ILLink analyzer #94858

Closed
stephentoub opened this issue Nov 16, 2023 · 7 comments · Fixed by #94888
Closed

Asserts in ILLink analyzer #94858

stephentoub opened this issue Nov 16, 2023 · 7 comments · Fixed by #94888
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers

Comments

@stephentoub
Copy link
Member

I don't know how to repro, but I was making some changes in System.Linq.csproj and all of a sudden was presented with a ton of these:
image

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 16, 2023
@ghost
Copy link

ghost commented Nov 16, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

I don't know how to repro, but I was making some changes in System.Linq.csproj and all of a sudden was presented with a ton of these:
image

Author: stephentoub
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@stephentoub stephentoub added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-Infrastructure-libraries labels Nov 16, 2023
@ghost
Copy link

ghost commented Nov 16, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

I don't know how to repro, but I was making some changes in System.Linq.csproj and all of a sudden was presented with a ton of these:
image

Author: stephentoub
Assignees: -
Labels:

untriaged, area-Tools-ILLink

Milestone: -

@sbomer
Copy link
Member

sbomer commented Nov 16, 2023

Thanks for reporting. It would be helpful if you could include the code changes you had made at the time of the assert (just the line numbers mentioned in the error should be enough).

  • If there's valid C# that can hit this assert, we need to fix the potential analyzer hole
  • If this was reached for invalid C# syntax while editing, we need to not fail in those cases

I'll experiment and see if I can find a way to hit this.

@sbomer sbomer self-assigned this Nov 16, 2023
@stephentoub
Copy link
Member Author

I was typing pretty fast and I don't know what was there at the time it triggered. Sorry :(

@stephentoub
Copy link
Member Author

If this was reached for invalid C# syntax while editing, we need to not fail in those cases

It very likely was invalid C# syntax; I was typing and editing and most likely in an inconsistent state.

@sbomer
Copy link
Member

sbomer commented Nov 16, 2023

What's confusing is that this code path should already be handling invalid C#. The assert indicates we were processing an assignment where the LHS was a BinaryOperation. I can't think of valid C# that does this. Code like a | b = 3 doesn't trigger it because there the LHS is an InvalidOperation that we already handle without asserting.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 17, 2023
@sbomer
Copy link
Member

sbomer commented Nov 17, 2023

I found a repro for this - code like a + = 3 has a BinaryOperation on the LHS, but a child of the BinaryOperation is InvalidOperation. Fixing it in #94888.

@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Nov 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants