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

Fix duplicate errors appended #2416

Open
wants to merge 1 commit into
base: main-powderhouse
Choose a base branch
from

Conversation

Keboo
Copy link
Member

@Keboo Keboo commented May 4, 2024

Fixes #2392.
This is a port from the fix on main. However due to additional work that is currently being done on Powerderhouse it is forcing the creation of the ArgumentConversionResult as part of parsing. This is due to the invocation SymbolResultTree.GetValueResultDictionary() which forces the evaluation of all ArgumentConversionResults. This is adding an additional test to make sure that the behavior is not re-introduced. This also fixes a few NRT issues and a comment typo.

Fixes dotnet#2392.
This is a port from the fix on main. However due to additional work that is currently being done on Powerderhouse it is forcing the creation of the ArgumentConversionResult as part of parsing. This is due to the invocation SymbolResultTree.GetValueResultDictionary() which forces the evaluation of all ArgumentConversionResults. This is adding an additional test to make sure that the behavior is not re-introduced. This also fixes a few NRT issues and a comment typo.
@KathleenDollard
Copy link
Contributor

I'd like to wait to approve this PR until we get the ValueSubsystem PR in, as it seems easier to reapply this one.

Also, I do not understand the change to ValueResult. The double call to GetArgumentConversionResult should not have caused any extra work as the result appears cached in a field and used by GetArgumentConversionResult.

I do not object to these changes, just want to understand this. I find this corner of the existing code confusing, at least in naming, and hope we can unwind some of that complexity after we have all the tests reinstated.

The other overall comment: I anticipated more changes in tests. Did we change the test where we discovered this problem in a different PR? (The test that had the false positive in relation to its name)

@KathleenDollard KathleenDollard added the Powderhouse Work to isolate parser and features label May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Powderhouse Work to isolate parser and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants