Runtime: Don't append local discriminators to locally defined type names. #14332
Conversation
include/swift/Demangling/Demangle.h
Outdated
| @@ -67,6 +68,7 @@ struct DemangleOptions { | |||
| Opt.ShortenThunk = true; | |||
| Opt.ShortenValueWitness = true; | |||
| Opt.ShortenArchetype = true; | |||
| Opt.ShowLocalDiscriminators = true; | |||
There was a problem hiding this comment.
Shouldn't this one be false, since ShowPrivateDiscriminators and DisplayModuleNames are also false?
There was a problem hiding this comment.
I think I would just pass -Xllvm -sil-full-demangle for that one test.
stdlib/public/runtime/Casting.cpp
Outdated
| @@ -100,6 +100,7 @@ static void _buildNameForMetadata(const Metadata *type, | |||
|
|
|||
| Demangle::DemangleOptions options; | |||
| options.QualifyEntities = qualified; | |||
| options.ShowLocalDiscriminators = false; | |||
There was a problem hiding this comment.
I think this one probably goes with qualified, like ShowPrivateDiscriminators. Because…
test/stdlib/StringDescribing.swift
Outdated
| expectEqual(String(describing: type(of: t)), "LocalType") | ||
| } | ||
|
|
||
| StringDescribingTestSuite.test("String(reflecting:) shouldn't include local discriminator") { |
There was a problem hiding this comment.
…String(reflecting:) should give a unique answer for every unique type.
There was a problem hiding this comment.
Should assertion message be reworded?
There was a problem hiding this comment.
Yes, like the one for private discriminators.
…mes. If a type was defined locally, a printed representation of an instance of that type would contain a local discriminator. Introducing an additional demangling option allows to opt out of this behaviour. Resolves: SR-6787.
|
@jrose-apple Addressed your comments. |
| let representation = String(reflecting: type(of: t)) | ||
|
|
||
| expectEqual(representation.prefix(6), "main.(") | ||
| expectEqual(representation.suffix(11), ").LocalType") |
There was a problem hiding this comment.
Ah, now we should be checking that the "#1" is there. Doesn't this fail?
There was a problem hiding this comment.
It doesn't fail. Isn't it expected behaviour for locally defined types?
There was a problem hiding this comment.
Hm, no. If String(reflecting:) uniquely identifies a type, that means it has to include the "#1". Maybe look for where else ShowPrivateDiscriminators is set to true? Or just combine the two settings, since I can't think why we'd ever want them to be different.
There was a problem hiding this comment.
It looks like after merging this change, the logic should be reconsidered.
There was a problem hiding this comment.
I don't think that affects this.
There was a problem hiding this comment.
Added a comment to the ticket.
|
There's been a change in the descriptor format and part of this was addressed by #12298. I'm going to close this out. Thank you for taking a look at this issue! |
If a type was defined locally, a printed representation of an instance of that
type would contain a local discriminator. Introducing an additional demangling
option allows to opt out of this behaviour.
Resolves SR-6787.