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

Variable misuse ML model found a bug where wrong variable is used to … #23437

Merged
merged 2 commits into from Nov 29, 2017

Conversation

heejaechang
Copy link
Contributor

…call a method.

Customer scenario

Customer clicks on error list to navigate to an error and VS crash.

Bugs this fixes

#23436

Workarounds, if any

There is no workaround.

Risk

Low risk. the code path is fallback code path, so not always exercised. and it will no longer throw an exception.

Performance impact

N/A

Is this a regression from a previous update?

No

Root cause analysis

when we try to navigate to an error, we try our best to go to right location. since error list can contain staled error, location info can be wrong, so we try to get right snapshot to calculate right location. but it is not always guaranteed that we can get to right text snapshot from roslyn snapshot. when that is failed, we fallback to whatever latest text snapshot we have to calculate location. here code used wrong snapshot when it is supposed to use current snapshot which lead to null ref.

How was the bug found?

Variable misuse ML model tool.

@heejaechang
Copy link
Contributor Author

@dotnet/roslyn-analysis @dotnet/roslyn-ide can you take a look?

@heejaechang
Copy link
Contributor Author

@jinujoseph is it for master? or should I target it to different branch?

@jinujoseph
Copy link
Contributor

This is for 15.6 preview2 , hence master is correct

@jinujoseph
Copy link
Contributor

cc @Pilchie for ask mode approval

@@ -362,7 +362,7 @@ private string GetTestOutputFilePath(string filepath)

try
{
outputFilePath = Path.GetDirectoryName(_filePath);
outputFilePath = Path.GetDirectoryName(filepath);
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this either a static method? Why can't it use the field instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it uses field such as Compilation, so can't be static. I don't know why it accepts filepath rather than not accepting any parameter and use all fields inside. but regardless, getting filepath is not a bug and more flexible if one want to reuse this method for different file path. so I am leaving that as it is.

@rchande
Copy link
Contributor

rchande commented Nov 28, 2017

@heejaechang Can you share more info about "variable misuse ML model"? Sounds interesting.

@jinujoseph
Copy link
Contributor

@rchande , i have fwd the mail thread for context

@rchande
Copy link
Contributor

rchande commented Nov 28, 2017

Thanks!

@heejaechang heejaechang merged commit f212118 into dotnet:master Nov 29, 2017
@rmarcil
Copy link

rmarcil commented Feb 14, 2018

Here's the accepted AI conference paper explaining this new "QA" tool that found this bug. Expect more AI improvements in SW in the future.

@Article{
allamanis2018learning,
title={Learning to Represent Programs with Graphs},
author={Miltiadis Allamanis, Marc Brockschmidt, Mahmoud Khademi},
journal={International Conference on Learning Representations},
year={2018},
url={https://openreview.net/forum?id=BJOFETxR-},
}

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.

None yet

5 participants