Improve diagnostic when decoding tuples but VT is missing by jcouv · Pull Request #23949 · dotnet/roslyn

jcouv · 2017-12-27T23:52:55Z

Customer scenario

Use a type that implements an interface that involves named tuples, but don't reference the assembly which defines ValueTuple types.
The interface that involves named tuples will be substituted with an error type with no details. This results in confusing error, such as error CS0648: '' is a type not supported by the language

But in some cases, the compilation should not even fail (if the interface doesn't end-up being used). And in some cases it should fail, but with an error reporting the missing ValueTuple type.

Bugs this fixes

Fixes #21727

Workarounds, if any

Add the missing reference. But the diagnostics don't offer much guidance...

Risk

Performance impact

Low. When decoding tuple names fails, use the partially decoded type (which has errors) rather than making a blank error type.

Is this a regression from a previous update?

No

Root cause analysis

Fairly edge case.

How was the bug found?

Reported by customer.

jcouv · 2017-12-28T17:56:20Z

@dotnet/roslyn-compiler for review. Thanks

jcouv · 2018-01-02T17:13:10Z

@dotnet/roslyn-compiler for review. Thanks

VSadov

LGTM

jcouv · 2018-01-02T22:18:10Z

@MeiChin-Tsai for ask-mode approval for 15.6. Thanks

AlekseyTs · 2018-01-03T00:16:36Z

Does VB have the same issue? #Closed

AlekseyTs · 2018-01-03T00:49:38Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/TupleTypeDecoder.cs

                if (!hasTupleElementNamesAttribute ||
-                    decoder._namesIndex == 0)
+                    decoder._namesIndex == 0 ||
+                    (object)decoded.VisitType(s_containsErrorPredicate, null) != null)


(object)decoded.VisitType(s_containsErrorPredicate, null) != null [](start = 20, length = 65)

I do not think this is the right way to fix the problem. We should be able to construct TupeTypeSymbol with the right names even when the definition of the underlying type is missing. It looks like the bug is in the private TypeSymbol DecodeType(TypeSymbol type) function below, it should call DecodeNamedType for SymbolKind.ErrorType and everything would just work as it should.
In addition, we probably should improve error recovery in this function for other situations, i.e., if decoding failed and metadataType already has a use-site error associated with it, we should return metadataType instead of losing the original error information by returning UnsupportedMetadataTypeSymbol. I believe, this change alone, would make the new unit-test to pass, but the decoding should succeed in this scenario and the unit-test should be adjusted to verify that. #Closed

@AlekseyTs I thought about making a TupleTypeSymbol with an underlying error type, but that felt riskier. I'll take another look and will push out to 15.7 depending on what I find.

As far as checking the use-site errors on metadataType, I think that is what I tried initially, but that created an unbounded recursion when compiling Roslyn with the bootstrap compiler. I didn't keep the precise stack trace.

That worked out. Error types have everything to pass for ValueTuples. The main thing is to not mangle the name.

AlekseyTs

Please consider an alternative way to address the problem - #23949 (comment)

AlekseyTs · 2018-01-03T00:52:48Z

Done with review pass (iteration 1). #Closed

MeiChin-Tsai · 2018-01-03T17:25:17Z

@jcouv please ping me again after you address Aleksey's change request. Thanks.

agocke · 2018-01-04T23:48:01Z

src/Compilers/VisualBasic/Portable/Symbols/SymbolExtensions.vb

+                Return DirectCast(substituted.GetMemberForDefinition(origMember), T)
+            End If
+
+            'Dim errorType = DirectCast(type, SubstitutedErrorType)


What's going on here? #Resolved

That's a WIP commit (to transfer from work to home). #Resolved

cston · 2018-02-01T17:15:33Z

src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/TupleTypeDecoder.vb

-                Case SymbolKind.ErrorType,
-                    SymbolKind.DynamicType,
+                Case SymbolKind.ErrorType
+                    Return DecodeNamedType(DirectCast(type, NamedTypeSymbol))


Is the same change needed in C#? #Resolved

The PR is marked for personal review. Still working on it. #Resolved

I'll be removing this change. #Resolved

jcouv · 2018-02-01T18:09:43Z

Done with removing unnecessary code.
@dotnet/roslyn-compiler for review. Thanks

obsolete

AlekseyTs

LGTM (iteration 8)

jcouv · 2018-02-02T04:16:28Z

Thanks.
@MeiChin-Tsai I'll go ahead and merge since this is no longer a 15.6 issue and my understanding is that compiler fixes for 15.7 and 15.8 do not require ask-mode approval at this stage.

jcouv added the Area-Compilers label Dec 27, 2017

jcouv added this to the 15.6 milestone Dec 27, 2017

jcouv self-assigned this Dec 27, 2017

jcouv force-pushed the tuple-decoding branch from 78d7730 to 3f12569 Compare December 28, 2017 00:39

Improve diagnostic when decoding tuples but VT is missing

967eb68

jcouv force-pushed the tuple-decoding branch from 3f12569 to 967eb68 Compare December 28, 2017 03:16

cston approved these changes Jan 2, 2018

View reviewed changes

VSadov approved these changes Jan 2, 2018

View reviewed changes

jcouv added the Pending director approval label Jan 2, 2018

AlekseyTs reviewed Jan 3, 2018

View reviewed changes

AlekseyTs previously requested changes Jan 3, 2018

View reviewed changes

jcouv removed the Pending director approval label Jan 3, 2018

Allow decoding of missing ValueTuple type as a tuple

bbfbdca

jcouv force-pushed the tuple-decoding branch from de39ab1 to 9f95fcb Compare January 3, 2018 08:58

Simplified implementation

244a800

jcouv force-pushed the tuple-decoding branch from 9f95fcb to 244a800 Compare January 3, 2018 08:59

Julien Couvreur added 2 commits January 3, 2018 14:12

Further simplification and adding tests

547ecd4

WIP on VB

3d1e111

jcouv force-pushed the tuple-decoding branch from 7bf0b27 to 3d1e111 Compare January 4, 2018 22:29

agocke reviewed Jan 4, 2018

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 5, 2018

jcouv added the 3 - Working label Jan 13, 2018

jcouv modified the milestones: 15.6, 15.7 Jan 17, 2018

Address PR feedback from Aleksey

0b96b54

jcouv requested a review from a team as a code owner February 1, 2018 00:07

jcouv added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed 3 - Working PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Feb 1, 2018

Cleanup

d69fbe5

cston reviewed Feb 1, 2018

View reviewed changes

Removing unnecessary change

a898462

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 1, 2018

AlekseyTs approved these changes Feb 1, 2018

View reviewed changes

cston approved these changes Feb 1, 2018

View reviewed changes

jcouv modified the milestones: 15.7, 15.8 Feb 2, 2018

jcouv merged commit 7064277 into dotnet:master Feb 2, 2018

jcouv deleted the tuple-decoding branch February 2, 2018 04:17

Conversation

jcouv commented Dec 27, 2017

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

jcouv commented Dec 28, 2017

Uh oh!

jcouv commented Jan 2, 2018

Uh oh!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 2, 2018

Uh oh!

AlekseyTs commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 3, 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!

jcouv Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MeiChin-Tsai commented Jan 3, 2018

Uh oh!

agocke Jan 4, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 5, 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!

cston Feb 1, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 1, 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!

jcouv Feb 1, 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!

jcouv commented Feb 1, 2018

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AlekseyTs commented Jan 3, 2018 •

edited

Loading

AlekseyTs Jan 3, 2018 •

edited

Loading

AlekseyTs commented Jan 3, 2018 •

edited

Loading

agocke Jan 4, 2018 •

edited by jcouv

Loading

jcouv Jan 5, 2018 •

edited

Loading

cston Feb 1, 2018 •

edited by jcouv

Loading

jcouv Feb 1, 2018 •

edited

Loading

jcouv Feb 1, 2018 •

edited

Loading