Fixed exception message in GetSemanticModel by svick · Pull Request #26659 · dotnet/roslyn

svick · 2018-05-05T22:52:30Z

For example, for this code:

var tree1 = SyntaxFactory.ParseSyntaxTree("");
var tree2 = SyntaxFactory.ParseSyntaxTree(@"class C
{
void M() {}
}", path: "C.cs");

var compilation = CSharpCompilation.Create(null).AddSyntaxTrees(tree1);

compilation.GetSemanticModel(tree2);

the exception changes from:

System.ArgumentException : SyntaxTree 'class C
{
    void M() {}
}' not found to remove
Parameter name: syntaxTree

to:

System.ArgumentException : SyntaxTree 'C.cs' not found
Parameter name: syntaxTree

Questions:

Is this the best way to display a SyntaxTree in an exception message?
I have also changed the name of the SyntaxTreeNotFoundTo resource and copied translations from the old resource to the new one (which also removed BOM from the translation files). Is that okay?

CyrusNajmabadi · 2018-05-07T04:33:57Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

                    }

-                    throw new ArgumentException(string.Format(CSharpResources.SyntaxTreeNotFoundTo, tree), $"{nameof(trees)}[{i}]");
+                    throw new ArgumentException(string.Format(CSharpResources.SyntaxTreeNotFoundToRemove, tree.FilePath), $"{nameof(trees)}[{i}]");


FilePath may be null, empty, or even duplicated across SyntaxTrees. It's a completely optional part of the whole API.

Right, but do you have a better way of identifying the syntax tree for diagnostic purposes?

Considering that the whole syntax tree can be hundreds of lines long, I think that's a much worse option. No information or ambiguous information still seems better to me than overwhelmingly huge amount of information for an exception message.

It could potentially be referred to by index.

What index? If you're talking about index within the current compilation, then I think that could make sense in other situations, but not here, where the exception is that the SyntaxTree is not contained in this compilation.

looking at the code, it should just be what it was before. i.e. $"{nameof(trees)}[{i}]". So it's telling you the index in the argument list where the problem tree occurs.

Ah, I understand what index you mean now. You're right that that's potentially useful information, but that's still passed as paramName to the exception (which means it will show up in its Message property), I didn't change that.

Are you saying that the message parameter should be just "SyntaxTree not found to remove", which would make the Message property e.g.:

SyntaxTree not found to remove
Parameter name: trees[4]

And how would this apply to the other cases I changed in this PR, where there is no index (since there is just one SyntaxTree)?

Are you saying that the message parameter should be just "SyntaxTree not found to remove",

That, or> SyntaxTree 'trees[4]' not found to remove"

And how would this apply to the other cases I changed in this PR, where there is no index

If it's just a single tree, then no need to print this information as it will be obvious from teh call.

@CyrusNajmabadi Done. I thought there's no need to repeat paramName in the message, so I went with the simpler version.

AlekseyTs · 2018-05-07T19:01:07Z

Does VB implementation have the same issue? #Resolved

svick · 2018-05-07T19:20:27Z

@AlekseyTs It does not. As far as I can see, VB does not check in GetSemanticModel that the tree belongs to the compilation. So, the following code does not throw:

var tree1 = SyntaxFactory.ParseSyntaxTree("");
var tree2 = SyntaxFactory.ParseSyntaxTree(@"
Class C
    Sub M()
    End Sub
End Class", path: "C.vb");

var compilation = VisualBasicCompilation.Create(null).AddSyntaxTrees(tree1);

compilation.GetSemanticModel(tree2);

Should I open an issue about that? #Resolved

AlekseyTs · 2018-05-07T19:23:22Z

Should I open an issue about that?

Yes, please open an issue. #Resolved

svick · 2018-05-07T19:41:17Z

@AlekseyTs Done: #26689. #Resolved

gafter · 2018-05-07T21:33:26Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>__arglist cannot have an argument passed by 'in' or 'out'</value>
  </data>
+  <data name="SyntaxTreeNotFound" xml:space="preserve">
+    <value>SyntaxTree '{0}' not found</value>


How about "SyntaxTree '{0}' is not part of the compilation"

@gafter Yeah, that's better. I have also changed SyntaxTreeNotFoundToRemove to "SyntaxTree is not part of the compilation, so it cannot be removed".

gafter

gafter · 2018-05-08T22:01:47Z

@AlekseyTs Do you have any further comments for this PR?

AlekseyTs · 2018-05-08T22:07:10Z

Do you have any further comments for this PR?

I didn't review changes in this PR and as of this moment it is not on my list to look at any time soon.

gafter · 2018-05-08T23:22:38Z

@dotnet/roslyn-compiler Can we please have a second review of this bug fix?

svick · 2018-05-09T01:10:57Z

The build failure is because I removed the format item from the message, but localized versions still use the old format strings, e.g.:

      <trans-unit id="SyntaxTreeNotFoundToRemove">
        <source>SyntaxTree is not part of the compilation, so it cannot be removed</source>
        <target state="needs-review-translation">No se encontró SyntaxTree '{0}' para quitarlo</target>
        <note />
      </trans-unit>

Should I change the xlf files like the one above to the untranslated state? E.g.

      <trans-unit id="SyntaxTreeNotFoundToRemove">
        <source>SyntaxTree is not part of the compilation, so it cannot be removed</source>
        <target state="new">SyntaxTree is not part of the compilation, so it cannot be removed</target>
        <note />
      </trans-unit>

jmarolf · 2018-05-09T01:29:57Z

@svick yes I would change them to an untranslated state with <target state="new"> so that they get re-translated on the next pass.

svick · 2018-05-09T10:56:25Z

@jmarolf Done.

jcouv · 2018-05-09T19:16:30Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

                    if (SyntaxAndDeclarationManager.IsLoadedSyntaxTree(tree, loadedSyntaxTreeMap))
                    {
-                        throw new ArgumentException(string.Format(CSharpResources.SyntaxTreeFromLoadNoRemoveReplace, tree), $"{nameof(trees)}[{i}]");
+                        throw new ArgumentException(string.Format(CSharpResources.SyntaxTreeFromLoadNoRemoveReplace), $"{nameof(trees)}[{i}]");


$"{nameof(trees)}[{i}]" [](start = 118, length = 23)

The extra arguments can be removed, for string where {0} was removed.

The remaining arguments are for the ArgumentException constructor, not for string.Format. Though the call to string.Format can be removed, so I just did that.

jcouv · 2018-05-09T19:17:56Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  </data>
-  <data name="SyntaxTreeNotFoundTo" xml:space="preserve">
-    <value>SyntaxTree '{0}' not found to remove</value>
+  <data name="SyntaxTreeNotFoundToRemove" xml:space="preserve">


Please find at least one test affected by each of the resource string changes and update the expectations.
(To do that, you run the test with an empty VerifyDiagnostics() and the test error will give you the text to copy/paste to update the baseline)

Those resource strings are exception messages, not diagnostics.

Should I test the messages by e.g. changing this test to the following?

AssertEx.Throws<ArgumentException>( () => comp.ReplaceSyntaxTree(newTree: SyntaxFactory.ParseSyntaxTree("Using System;"), oldTree: t1), e => Assert.Equal(CSharpResources.SyntaxTreeNotFoundToRemove, e.Message));

Doing that doesn't really seem worth it to me.

Or am I missing something and there is a way to test exception messages using VerifyDiagnostics?

I think it's fine to leave as-is, given that it's for exceptions not diagnostics.

jcouv

Done with review pass (iteration 6).

jcouv · 2018-05-09T20:08:44Z

@svick Thanks for the clarifications. Makes sense

jcouv

LGTM

gafter · 2018-05-10T02:04:02Z

@svick Thank you for the contribution. You are awesome!

* dotnet/master: (732 commits) Disable procdump on Spanish runs use named args. Exclude System.Diagnostics.DiagnosticSource from PortableFacades Exclude System.Net.Http from PortableFacades Align the implementations of IAssemblySymbol.GivesAccessTo (#26727) Fixed exception message in GetSemanticModel (#26659) Implement IDiscardSymbol.Equals and GetHashCode (#26720) Produce correct diagnostics on implicit operator conversions for stackalloc expressions (#26248) Remove special case handling of CreateDiagnosticProviderAndFixer throwing an exception Add verbose logging to DevDivInsertionFiles if you want it Exclude Microsoft.Composition as something we build a dependency map for Fix the message for Make Field Readonly Update LangVersion to 7.3 (#26698) fix extension deployment and add explainatory comment Add TaskExtensions to compiler package Add System.Diagnostics.DiagnosticSource to SignToolData.json Include missing facade: System.Diagnostics.DiagnosticSource Use default expressions. Remove forwarding method. Revert project chnage. ...

svick added 4 commits May 5, 2018 22:47

Fixed exception message in GetSemanticModel

4156edf

Better name for exception message resource

071849f

Brought back translations for renamed message

03b652d

Use FilePath in exception messages for SyntaxTree

a445d41

svick requested a review from a team as a code owner May 5, 2018 22:52

jcouv added the Area-Compilers label May 6, 2018

jaredpar added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 7, 2018

CyrusNajmabadi reviewed May 7, 2018

View reviewed changes

gafter reviewed May 7, 2018

View reviewed changes

gafter self-assigned this May 7, 2018

gafter added this to the 15.8 milestone May 7, 2018

gafter approved these changes May 8, 2018

View reviewed changes

Addressed review comments

3e2074a

Change SyntaxTreeNotFoundToRemove to untranslated

5132719

jcouv reviewed May 9, 2018

View reviewed changes

Removed unnecessary calls to string.Format

2f113ec

jcouv approved these changes May 9, 2018

View reviewed changes

gafter merged commit 9c9e6ca into dotnet:master May 10, 2018

gafter mentioned this pull request May 10, 2018

Calling GetSemanticModel with the wrong tree resulting in confusing exception message #26658

Closed

svick deleted the getsemanticmodel-message branch May 10, 2018 10:48

Conversation

svick commented May 5, 2018

Uh oh!

CyrusNajmabadi May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 7, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svick commented May 7, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 7, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svick commented May 7, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented May 8, 2018

Uh oh!

AlekseyTs commented May 8, 2018

Uh oh!

gafter commented May 8, 2018

Uh oh!

svick commented May 9, 2018

Uh oh!

jmarolf commented May 9, 2018

Uh oh!

svick commented May 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 9, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

CyrusNajmabadi May 7, 2018 •

edited

Loading

AlekseyTs commented May 7, 2018 •

edited by gafter

Loading

svick commented May 7, 2018 •

edited by gafter

Loading

AlekseyTs commented May 7, 2018 •

edited by gafter

Loading

svick commented May 7, 2018 •

edited by gafter

Loading