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

[SR-12248] <unknown> diagnostic location regarding Codable derived conformances #54675

Closed
dan-zheng opened this issue Feb 21, 2020 · 15 comments
Closed
Labels
compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation good first issue Good for newcomers improvement type checker Area → compiler: Semantic analysis

Comments

@dan-zheng
Copy link
Collaborator

Previous ID SR-12248
Radar rdar://problem/59655704
Original Reporter @dan-zheng
Type Improvement
Status Closed
Resolution Done

Attachment: Download

Environment

Current apple/swift master: 9600b97

Additional Detail from JIRA
Votes 2
Component/s Compiler
Labels Improvement, DiagnosticsQoI, StarterBug, TypeChecker
Assignee hassaneldesouky (JIRA)
Priority Medium

md5: 248ea873711e653b56c1bb05a0820282

Issue Description:

struct Foo: Codable {
  var x: Int
  var x: Int
}
$ swift sr-12248.swift
sr-12248.swift:3:7: error: invalid redeclaration of 'x'
  var x: Int
      ^
sr-12248.swift:2:7: note: 'x' previously declared here
  var x: Int
      ^
<unknown>:0: error: invalid redeclaration of 'x'
<unknown>:0: note: 'x' previously declared here
sr-12248.swift:1:8: error: type 'Foo' does not conform to protocol 'Encodable'
struct Foo: Codable {
       ^
Swift.Encodable:2:10: note: protocol requires function 'encode(to:)' with type 'Encodable'
    func encode(to encoder: Encoder) throws
         ^
sr-12248.swift:3:7: note: cannot automatically synthesize 'Encodable' because '<<error type>>' does not conform to 'Encodable'
  var x: Int
      ^
sr-12248.swift:3:7: note: cannot automatically synthesize 'Encodable' because '<<error type>>' does not conform to 'Encodable'
  var x: Int
      ^

I suspect that the "<unknown>" source locations come from declarations synthesized by Codable derived conformances. This issue seems low-priority.

@hborla
Copy link
Member

hborla commented Feb 21, 2020

@swift-ci create

@CodaFi
Copy link
Member

CodaFi commented Feb 25, 2020

For anybody that wants to take this up, the place where we're diagnosing this is currently way down in `checkRedeclaration` which resides in TypeCheckDeclPrimary. Look for the reference to `diag::invalid_redecl`. Above that reference, you'll notice we're checking to see if the decl is an implicit initializer, then emitting a specialized diagnostic. The same thing needs to be done, but for non-initializers. Add another branch above this one and check if `current` is implicit, then emit a specialized diagnostic that calls out the fact that the variable is synthesized. Make sure not to diagnose the synthesized ValueDecl itself or there won't be a valid source location to attach the diagnostic to - you can probably diagnose the enclosing declaration context for that declaration.

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

@CodaFi Is this bug has been resolved? Because I can't reproduce the bug.

@CodaFi
Copy link
Member

CodaFi commented Mar 27, 2020

hassaneldesouky (JIRA User) I have a master compiler and I still get it. Perhaps something else is going wrong before you're able to see this diagnostic.

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

@CodaFi this what happens when I run the test provided in the description

@CodaFi
Copy link
Member

CodaFi commented Mar 27, 2020

You're not using the just-built swift in that command line (unless you happen to have your environment setup to point into your build directory). This does not reproduce on a 5.1 compiler.

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

Thank you for making me notices that!

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

@CodaFi This is completely not related to the issue but I'm not able to build the compiler (master) branch.
I'm using Ubuntu and building with `ninja swift`

Here's the error:

/home/hassaneldesouky/Desktop/swift-source/swift/lib/IRGen/IRGenDebugInfo.cpp:663:78: error: too few arguments to function call, expected 5, have 4
        DBuilder.createModule(Parent, Name, ConfigMacros, RemappedIncludePath);
        ~~~~~~~~~~~~~~~~~~~~~                                                ^
/home/hassaneldesouky/Desktop/swift-source/llvm-project/llvm/include/llvm/IR/DIBuilder.h:741:5: note: 'createModule' declared here
    DIModule *createModule(DIScope *Scope, StringRef Name,
    ^
/home/hassaneldesouky/Desktop/swift-source/swift/lib/IRGen/IRGenDebugInfo.cpp:1718:38: error: too many arguments to function call, expected at most 13, have 15
      /* RangesBaseAddress */ false, Sysroot, SDK);
                                     ^~~~~~~~~~~~
/home/hassaneldesouky/Desktop/swift-source/llvm-project/llvm/include/llvm/IR/DIBuilder.h:138:5: note: 'createCompileUnit' declared here
    DICompileUnit *
    ^
2 errors generated.

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

@CodaFi I reverted to an older commit and managed to build. Anyways, can you please explain what do you mean by the sentence "Make sure not to diagnose the synthesized ValueDecl itself or there won't be a valid source location to attach the diagnostic to - you can probably diagnose the enclosing declaration context for that declaration."?

I tried adding an if statement to check `if (current->isImplicit())` but I found out this is wrong/ incomplete.

@CodaFi
Copy link
Member

CodaFi commented Mar 30, 2020

Seems like you have a mismatched LLVM-project checkout. This happens from time-to-time (when we merge master-next to master, for example). Just run

utils/update-checkout --clone

and rebuild with

--reconfigure 

and everything'll work out.

@CodaFi
Copy link
Member

CodaFi commented Mar 30, 2020

To the second question: When you use the decl-based diagnostics entry point, if you try to diagnose an implicit decl you'll come up with a diagnostic that has an invalid source location (because there's literally no location in source for the declaration!). The idea was that if you detect such a case, you should walk to the nearest non-synthesized declaration context and diagnose that instead.

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 5, 2020

Comment by Hassan ElDesouky (JIRA)

@CodaFi Thank you so much for helping me build the project. Everything is working fine right now!

I understand what is the problem now. However, I have two inquiries:

  1. I'm trying to get the "enclosing declaration context for that declaration" and tried the following but when I diagnose this it is still giving me an invalid source location.
  • TypeCheckAttr#L1335

    auto currentEnclosingDecl = current->getDeclContext()->getInnermostDeclarationDeclContext(); 
    currentEnclosingDecl->diagnose() // Still <unknown> source location.

    I tried a lot of variations for this but didn't work out, so what I'm doing wrong?

2. When debugging this part of code I found out an interesting thing for me and it's... This `else` block is always getting called first and I don't know why![]( Even after I put another if branch to check to if the current->isImplecit() above checking the other initializers... Again this else block gets called first)
TypeCheckDeclPrimary#L714

@LucianoPAlmeida
Copy link
Collaborator

hassaneldesouky (JIRA User) I think a simple replace of the else clause here TypeCheckDeclPrimary#L714 for something like `else if (![](current->isImplicit() && )other->isImplicit())` should fix the problem
I'm not sure but what I think is happening is when Foo conforms to Codable, the compiler synthesizes declarations for CodingKeys 'x' and those are the declarations that are emitting an unknown location because they are implicit, not actually on code. So that should solve the issue because it should not diagnose redeclaration for implicit 'Foo.CodingKeys.x' synthesized decls. That's just a thought, maybe a hint to solve this ... I didn't actually tested all cases, but maybe this can help 🙂

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 5, 2020

Comment by Hassan ElDesouky (JIRA)

Thank you so much man! I’ll try to use this ❤️

@swift-ci
Copy link
Collaborator

Comment by Hassan ElDesouky (JIRA)

PR is merged #31037

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation good first issue Good for newcomers improvement type checker Area → compiler: Semantic analysis
Projects
None yet
Development

No branches or pull requests

5 participants