[Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type #11609

Kacper20 · 2017-08-24T21:53:54Z

The intended behavior is described in JIRA issue.
Resolves SR-5324.

It's my first touch to the Swift compiler and I'm not sure whether it's the right approach. I'm looking for your feedback :)
Could you take a look @xedin ?

Thanks!

xedin

@Kacper20 Thanks! The changes make sense to me!

Here is a couple of minor notes:

please prefix commit message with [Diagnostics] and Resolves: SR-5324..
add test-case from SR-5324 to the end of test/Constraints/members.swift so we have a way to make sure that it doesn't regress in the future.

xedin · 2017-08-24T22:05:55Z

It would be great if you could also come up with a couple more similar test-cases so we could add them into the suite.

Kacper20 · 2017-08-24T22:09:50Z

Of course, I will work on it right away!

Thank you

slavapestov · 2017-08-24T22:45:18Z

lib/Sema/CSDiag.cpp

+      auto memberTypeContext = member->getDeclContext()->getInnermostTypeContext();
+      auto currentTypeContext = CS.DC->getInnermostTypeContext();
+      if (memberTypeContext->getSyntacticDepth() < currentTypeContext->getSyntacticDepth()) {
+        diagnose(loc, diag::could_not_use_instance_member_of_outer_type_on_nested_type,


I'd rather not have a new diagnostic here. Can we emit the existing diagnostic, and have the fixit fully qualify the type name instead?

Thanks @slavapestov!
Could you explain the reason? I'd like to understand pros and cons of doing this.
Do I understand correctly that you suggest in this case:

struct Outer { var outer: Int struct Inner { var inner: Int func sum() -> Int { return inner + outer } } }

emit error:

instance member 'outer' cannot be used on type 'Outer.Inner'

instead of current(reported in issue):

instance member 'outer' cannot be used on type 'Outer'

Also: what do you mean by fixit? I think that compiler doesn't generate fixit in this case, at least I don't see it in Xcode.

Thanks for the help!

I think what @slavapestov is trying to say is that we should fix the argument of the existing diagnostic diag::could_not_use_instance_member_on_type to say 'Outer.Inner' vs. only 'Outer' right now (first argument instanceType) instead of creating new diagnostic with the same text, because it's already reported right after special cased one.

@xedin Yeah, that’s understandable but new diagnostic doesn’t have the same text. It has different one, with different number of template arguments(3 in this case). With new one there is following message: instance member 'outer' of type 'Outer' cannot be accessed on instance of nested type 'Outer.Inner. This gives more info than instance member ‘outer’ cannot be used on type ‘Outer.Inner(about the variable definition scope). If we want to have this exact message(like specified in JIRA ticket) we need to have a new diagnostic. If it’s not necessary I could of course change it and implement it as @slavapestov suggests. What do you think?

@Kacper20 There is a way to make existing one to distinguish between on type and on nested type, you can add on %select{type|nested type}2 and pass the flag as a third argument (please take a look at how other diagnostics use it in DiagnosticsSema.def).

Thanks @xedin. I believe the same could be done with of type ‘Outer’, which is also additional? Don’t know how clear will it be but let’s find out :) That’s a great tip, will look at it.

CodaFi · 2017-08-26T20:21:51Z

lib/Sema/CSDiag.cpp

+      // provide more specialized message
+      auto memberTypeContext = member->getDeclContext()->getInnermostTypeContext();
+      auto currentTypeContext = CS.DC->getInnermostTypeContext();
+      if (memberTypeContext->getSyntacticDepth() < currentTypeContext->getSyntacticDepth()) {


Syntactic depth will catch this for explicit nesting, but what about extensions of nested types?

Good catch! I will check it, thank you!

I've checked it (sorry for the delay). In case of extensions it doesn't even go to this path of code. In case of writing code like:

struct Outer { var outer: Int struct Inner { var inner: Int } } extension Outer.Inner { func sum() -> Int { return inner + outer } }

message is use of unresolved identifier 'outer'. Nevertheless, I've changed check to use SemanticDepth instead. Thanks!

CodaFi · 2017-08-26T20:22:26Z

lib/Sema/CSDiag.cpp

      diagnose(loc, diag::could_not_use_instance_member_on_type,
               instanceTy, memberName)
-        .highlight(baseRange).highlight(nameLoc.getSourceRange());


Nit: Restore the formatting here

CodaFi · 2017-08-26T20:23:56Z

lib/Sema/CSDiag.cpp

+        diagnose(loc, diag::could_not_use_instance_member_of_outer_type_on_nested_type,
+                 memberTypeContext->getDeclaredTypeOfContext(), memberName,
+                 currentTypeContext->getDeclaredTypeOfContext()
+         ).highlight(baseRange).highlight(nameLoc.getSourceRange());


Nit: This is stdlib-style formatting, not Swift style. Please move the dangling paren to the end of the previous line and indent the member call like highlight below.

slavapestov · 2017-08-27T03:57:27Z

@Kacper20 Check out this PR -- #11640 -- do you think the logic can be combined somehow?

Kacper20 · 2017-08-29T09:25:13Z

@slavapestov I'm not sure about this, I think I lack the knowledge of how those two relate to each other. Could you please check my last comment to your review? I've asked some questions related to this PR and I believe that answers would allow me to complete this feature(if introducing new diagnostic is not a good idea).

Thanks in advance!

CodaFi · 2017-09-18T14:31:45Z

@swift-ci please smoke test

Kacper20 · 2017-09-18T15:26:36Z

Sorry for the delay! :)
@slavapestov I've managed to preserve the desired message without emitting new kind of diagnostic. Could you please take a look? This required some changes on the other part of code (~5040L, CSDiag.cpp) and it makes code there a bit uglier there.
Test case is written in members.swift.
@xedin Could you take a look, too?

xedin

LGTM!

xedin · 2017-09-18T18:24:01Z

@swift-ci please smoke test

xedin · 2017-09-18T19:02:39Z

@Kacper20 Looks like this improve a bunch of other tests, so you'll have to fix that and introduced couple of new crashers which need to be addressed before we merge this, do you know how to run test suite locally?

Kacper20 · 2017-09-18T19:08:55Z

@xedin Yes, will fix it and let you know. I have some troubles running specific tests(lit.py failing), but will do it with full test suite.

xedin · 2017-09-18T19:11:44Z

@Kacper20 Thanks!

…r type references instance member of outer type

Kacper20 · 2017-09-19T12:09:54Z

Should be fixed right now.

xedin · 2017-09-19T18:28:31Z

@swift-ci please smoke test

Kacper20 · 2017-09-19T19:35:55Z

@xedin I think that this failure on Linux is unrelated to my changes.

xedin · 2017-09-19T19:36:46Z

Yeah, I had that happen yesterday too, let's try a clean Linux build.

xedin · 2017-09-19T19:36:56Z

@swift-ci please clean smoke test Linux platform

xedin · 2017-09-19T19:40:56Z

@Kacper20 I'm going to squash your changes before merging if you don't mind?

Kacper20 · 2017-09-19T19:43:18Z

@xedin Of course, thanks!

xedin · 2017-09-19T21:08:51Z

Merged, thanks for helping out with this, @Kacper20!

Kacper20 · 2017-09-19T21:11:27Z

Thanks for all of the help in implementing this @xedin, @slavapestov, @CodaFi!

xedin requested changes Aug 24, 2017

View reviewed changes

slavapestov reviewed Aug 24, 2017

View reviewed changes

CodaFi reviewed Aug 26, 2017

View reviewed changes

slavapestov mentioned this pull request Aug 27, 2017

[Qol]Fixit nested static value #11640

Merged

Kacper20 force-pushed the sr_5324 branch 2 times, most recently from b5c0e34 to 68ca0e5 Compare September 18, 2017 14:09

Kacper20 changed the title ~~Improved diagnostic when instance member of outer type is referenced from nested type~~ [Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type Sep 18, 2017

xedin approved these changes Sep 18, 2017

View reviewed changes

Kacper20 and others added 5 commits September 19, 2017 13:15

[Diagnostics] Resolves: SR-5324. Improved error diagnostics when inne…

1331d35

…r type references instance member of outer type

Changed check from using SyntacticDepth to SemanticDepth

278b43d

Preserving the behavior without emitting new diagnostic implementation

68d4b9c

Reference to JIRA issue inserted in tests as a comment

f06e07c

Test fixes

b04c70a

Kacper20 force-pushed the sr_5324 branch from 7f02f78 to b04c70a Compare September 19, 2017 12:06

xedin merged commit 3902688 into apple:master Sep 19, 2017

swift-ci mentioned this pull request Sep 18, 2017

[SR-5324] Better diagnostic when instance member of outer type is referenced from nested type #47898

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type #11609

[Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type #11609

Kacper20 commented Aug 24, 2017

xedin left a comment

xedin commented Aug 24, 2017

Kacper20 commented Aug 24, 2017

slavapestov Aug 24, 2017

Kacper20 Aug 26, 2017 •

edited

xedin Aug 29, 2017

Kacper20 Aug 29, 2017 •

edited

xedin Aug 30, 2017

Kacper20 Aug 30, 2017

CodaFi Aug 26, 2017

Kacper20 Aug 29, 2017

Kacper20 Sep 18, 2017

CodaFi Aug 26, 2017

CodaFi Aug 26, 2017 •

edited

slavapestov commented Aug 27, 2017

Kacper20 commented Aug 29, 2017

CodaFi commented Sep 18, 2017

Kacper20 commented Sep 18, 2017 •

edited

xedin left a comment

xedin commented Sep 18, 2017

xedin commented Sep 18, 2017

Kacper20 commented Sep 18, 2017

xedin commented Sep 18, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

xedin commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

[Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type #11609

[Diagnostics] Resolves: SR-5324 Improved diagnostic when instance member of outer type is referenced from nested type #11609

Conversation

Kacper20 commented Aug 24, 2017

xedin left a comment

Choose a reason for hiding this comment

xedin commented Aug 24, 2017

Kacper20 commented Aug 24, 2017

Choose a reason for hiding this comment

Kacper20 Aug 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kacper20 Aug 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi Aug 26, 2017 • edited

Choose a reason for hiding this comment

slavapestov commented Aug 27, 2017

Kacper20 commented Aug 29, 2017

CodaFi commented Sep 18, 2017

Kacper20 commented Sep 18, 2017 • edited

xedin left a comment

Choose a reason for hiding this comment

xedin commented Sep 18, 2017

xedin commented Sep 18, 2017

Kacper20 commented Sep 18, 2017

xedin commented Sep 18, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

xedin commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

xedin commented Sep 19, 2017

Kacper20 commented Sep 19, 2017

Kacper20 Aug 26, 2017 •

edited

Kacper20 Aug 29, 2017 •

edited

CodaFi Aug 26, 2017 •

edited

Kacper20 commented Sep 18, 2017 •

edited