Split Symbol.swift to reduce complexity of the file #8

Kyle-Ye · 2021-12-11T08:58:04Z

The Symbol.swift currently has 1400 lines.

This PR is intended to split the Symbol.swift file into a file system hierarchy.

So that the Symbol.swift only have SymbolGraph.Symbol definition and also makes it easier for writing test case.

Discussion

Which style do you prefer?

- Symbol
	- Availability
		- Availability.swift
		- Domain.swift
	- Symbol.swift

Or

- Symbol
	- Availability
		- Domain.swift
	- Availability.swift
- Symbol.swift

What should we do if there are 2 struct/enum both named as "Kind"

Currently there are SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind and SymbolGraph.Symbol.Kind.

Although they are not in the same folder, they are in the same module. Which is not allowed by Swift

error: filename "Kind.swift" used twice: 'Symbol/DeclarationFragments/Fragment/Kind.swift' and 'SymbolKit/SymbolGraph/Symbol/Kind.swift'
note: filenames are used to distinguish private declarations with the same name

https://belkadan.com/blog/2021/11/Swift-Mangling-Regret-Private-Discriminators/

Status

~~WIP~~
After the above 2 discussion is solved, I'll complete the full PR.

franklinsch · 2021-12-13T12:54:13Z

Thanks for taking this initiative, @Kyle-Ye! I agree, the Symbol.swift file is quite long and becoming hard to navigate. I personally prefer the first approach you propose. I also don't think that the type hierarchy necessarily needs to be reflected in the directory structure. I think the organization you're proposing makes sense though.

Regarding the collision of Kind, one convention I've seen is adding a + in the file name for extensions, for example Fragment+Kind.swift and Symbol+Kind.swift. To follow this convention pedantically, we'd have SymbolGraph+Symbol+Kind.swift, SymbolGraph+Symbol+Identifier.swift etc., but I think the directory structure you're proposing is better suited.

Kyle-Ye · 2021-12-13T16:57:03Z

Thanks for taking this initiative, @Kyle-Ye! I agree, the Symbol.swift file is quite long and becoming hard to navigate. I personally prefer the first approach you propose. I also don't think that the type hierarchy necessarily needs to be reflected in the directory structure. I think the organization you're proposing makes sense though.

Regarding the collision of Kind, one convention I've seen is adding a + in the file name for extensions, for example Fragment+Kind.swift and Symbol+Kind.swift. To follow this convention pedantically, we'd have SymbolGraph+Symbol+Kind.swift, SymbolGraph+Symbol+Identifier.swift etc., but I think the directory structure you're proposing is better suited.

Got it. Maybe we could combine them to be the following:

For node struct: parent/node/node.swift
For leaf struct: parent/parent+leaf.swift

Node struct means it has other struct defined in its namespace

Thus, we got:

├── Availability
│   ├── Availability+AvailabilityItem.swift
│   ├── Availability+Domain.swift
│   └── Availability.swift
├── DeclarationFragments
│   ├── DeclarationFragments.swift
│   └── Fragment
│       ├── Fragment+Kind.swift
│       └── Fragment.swift
├── FunctionSignature
│   ├── FunctionSignature+FunctionParameter.swift
│   └── FunctionSignature.swift
├── Symbol+AccessControl.swift
├── Symbol+Identifier.swift
├── Symbol+Kind.swift
├── Symbol+SPI.swift
└── Symbol.swift

What's your opinion about it? @franklinsch

franklinsch · 2021-12-13T17:02:04Z

Makes sense to me! I'll defer to @QuietMisdreavus to do a review as well.

QuietMisdreavus · 2021-12-13T18:02:24Z

Thanks so much for this! I like the organization you and Franklin have agreed upon; i think it makes working with the Symbol-related types much cleaner. Consider this my thumbs-up to keep going.

Kyle-Ye · 2021-12-14T05:40:58Z

@swift-ci Please test

Kyle-Ye · 2021-12-14T05:43:03Z

Complete the full PR.

It is a little big PR to review. But actually only involves moving files around.

Could you please help review it in your spare time? @franklinsch @QuietMisdreavus ❤️

Kyle-Ye · 2021-12-14T05:55:25Z

I tried many ways, but I can't mark Relationship.swift as "rename" instead of "delete" and "add". Whether I use "git mv", first modify then mv nor first mv then modify can not achieve it.

Maybe the only way is to do 2 separate commits😂. (After doing 2 separate commits to achieve this, if I do squash commits, it will change to delete and add again😵‍💫)

Kyle-Ye · 2021-12-14T15:33:49Z

./Sources
└── SymbolKit
    ├── Mixin.swift
    ├── SymbolGraph
    │   ├── LineList
    │   │   ├── LineList.swift
    │   │   └── SourceRange
    │   │       ├── SourceRange+Position.swift
    │   │       └── SourceRange.swift
    │   ├── Relationship
    │   │   ├── Relationship+Kind.swift
    │   │   ├── Relationship+SourceOrigin.swift
    │   │   ├── Relationship.swift
    │   │   └── Swift
    │   │       ├── Swift+GenericConstraints.swift
    │   │       └── Swift.swift
    │   ├── Symbol
    │   │   ├── Availability
    │   │   │   ├── Availability+AvailabilityItem.swift
    │   │   │   ├── Availability+Domain.swift
    │   │   │   └── Availability.swift
    │   │   ├── DeclarationFragments
    │   │   │   ├── DeclarationFragments.swift
    │   │   │   └── Fragment
    │   │   │       ├── Fragment+Kind.swift
    │   │   │       └── Fragment.swift
    │   │   ├── FunctionSignature
    │   │   │   ├── FunctionSignature+FunctionParameter.swift
    │   │   │   └── FunctionSignature.swift
    │   │   ├── Swift
    │   │   │   ├── Namespace.swift
    │   │   │   ├── Swift+Extension.swift
    │   │   │   ├── Swift+GenericConstraint.swift
    │   │   │   ├── Swift+GenericParameter.swift
    │   │   │   └── Swift+Generics.swift
    │   │   ├── Symbol+AccessControl.swift
    │   │   ├── Symbol+Identifier.swift
    │   │   ├── Symbol+Kind.swift
    │   │   ├── Symbol+KindIdentifier.swift
    │   │   ├── Symbol+Location.swift
    │   │   ├── Symbol+Mutability.swift
    │   │   ├── Symbol+Names.swift
    │   │   ├── Symbol+SPI.swift
    │   │   └── Symbol.swift
    │   ├── SymbolGraph+Metadata.swift
    │   ├── SymbolGraph+Module.swift
    │   ├── SymbolGraph+OperatingSystem.swift
    │   ├── SymbolGraph+Platform.swift
    │   ├── SymbolGraph+SemanticVersion.swift
    │   └── SymbolGraph.swift
    ├── SymbolKit.docc
    │   ├── Resources
    │   │   ├── conforms.png
    │   │   ├── member.png
    │   │   └── twonodes.png
    │   └── SymbolKit.md
    └── UnifiedSymbolGraph
        ├── GraphCollector.swift
        ├── UnifiedSymbol.swift
        └── UnifiedSymbolGraph.swift

QuietMisdreavus · 2021-12-14T22:26:23Z

The changes look good. Are you using an automatic formatter/linter? I noticed that the public keywords on the types got moved in favor of the extension blocks. Since the rest of the swift-docc repos tend to have their keywords the other way i wanted to make sure.

Kyle-Ye · 2021-12-15T02:51:29Z

The changes look good. Are you using an automatic formatter/linter? I noticed that the public keywords on the types got moved in favor of the extension blocks. Since the rest of the swift-docc repos tend to have their keywords the other way i wanted to make sure.

Yes, I use an automatic formatter to use the public extension style which is my favor.

Since it was not the way swift-docc related repos' style, maybe I should change them back into the original style

extension XX {
	public struct YY {
	}
}

QuietMisdreavus · 2021-12-15T15:26:49Z

The changes look good. Are you using an automatic formatter/linter? I noticed that the public keywords on the types got moved in favor of the extension blocks. Since the rest of the swift-docc repos tend to have their keywords the other way i wanted to make sure.

Yes, I use an automatic formatter to use the public extension style which is my favor.

Since it was not the way swift-docc related repos' style, maybe I should change them back into the original style
extension XX {
	public struct YY {
	}
}

That makes sense. I think we should stick to the existing style, to remove some churn. (As much as we can with a change like this! 😅)

Kyle-Ye · 2021-12-15T16:21:42Z

The changes look good. Are you using an automatic formatter/linter? I noticed that the public keywords on the types got moved in favor of the extension blocks. Since the rest of the swift-docc repos tend to have their keywords the other way i wanted to make sure.

Yes, I use an automatic formatter to use the public extension style which is my favor.
Since it was not the way swift-docc related repos' style, maybe I should change them back into the original style
extension XX {
	public struct YY {
	}
}
That makes sense. I think we should stick to the existing style, to remove some churn. (As much as we can with a change like this! 😅)

Done👌

QuietMisdreavus

This looks good, thanks so much! Can you rebase and run tests before merging?

Separate into 2 commits because git can't mark it as rename since the change is too big

Kyle-Ye · 2021-12-16T04:43:27Z

@swift-ci Please test

Kyle-Ye · 2021-12-16T04:47:05Z

This looks good, thanks so much! Can you rebase and run tests before merging?

Done. I think we should defer to @franklinsch 's approval before merging.

QuietMisdreavus · 2021-12-16T15:54:58Z

This looks good, thanks so much! Can you rebase and run tests before merging?

Done. I think we should defer to @franklinsch 's approval before merging.

Franklin's on vacation until January; are you comfortable keeping this open until then, or would you like me to pull in some other reviewers so we can land it sooner? I don't want to make you rebase this over and over if anything else lands before then, since it's a larger change that i feel is worthwhile to land.

Kyle-Ye · 2021-12-16T16:05:11Z

This looks good, thanks so much! Can you rebase and run tests before merging?

Done. I think we should defer to @franklinsch 's approval before merging.

Franklin's on vacation until January; are you comfortable keeping this open until then, or would you like me to pull in some other reviewers so we can land it sooner? I don't want to make you rebase this over and over if anything else lands before then, since it's a larger change that i feel is worthwhile to land.

Maybe we could invite some other reviewers? +1 for this

Kyle-Ye · 2021-12-16T16:17:38Z

Hello @ethan-kusters , would you please help review this MR? Thanks

ethan-kusters

This looks great to me! Thank you @Kyle-Ye. I think it makes the codebase much more approachable.

I also generated a DocC archive before and after this change and then diffed them to confirm there's no change in public API here. Looks all good. 👍🏻

ethan-kusters · 2021-12-18T00:45:02Z

I think we should consider cherry-picking this change onto the release/5.6 branch after landing it here since this is low-risk and it will be difficult to manage merge conflicts otherwise.

WowbaggersLiquidLunch · 2022-01-11T15:57:13Z

Sorry for reviving the discussion long after the PR is merged.

I find the prevalent use of "ParentType+" prefixes in file names makes the repository a little difficult to navigate. For example, in this screenshot of part of the repository structure in Xcode's sidebar:

Because of the shared prefixes, it becomes difficult to tell apart the files when their names are long (and the prefixes contribute to the file name lengths). This is can become much worse with types introduced in #12, which can lead to file names like SymbolGraph+SemanticVersion+Prerelease+Identifier.

I'd like to open a PR that amends the naming convention to the following, but would like to discuss it here first:

Keep the "+" convention for protocol conformances only, like in swift-collections.

Group files that define types under the same immediate parent types into the same directory with the file that defines the parent type, and do not repeat the parent type's name. <- It's a mouthful, but this is what I mean:

For a type structure like this:

struct S {
    struct A {
        struct X {}
        struct Y {}
    }
    struct B {
        struct W {
            struct T {}
        }
        struct X {}
        struct Z {}
    }
}

extension Y: Equatable {}

If we separate each type into its own file, then the directory structure should be like this:

S
├─ S.swift
├─ A
│  ├─ A.swift
│  ├─ X.swift
│  └─ Y
│     ├─ Y.swift
│     └─ Y+Equatable.swift
└─ B
   ├─ B.swift
   ├─ W
   │  ├─ W.swift
   │  └─ T.swift
   ├─ X.swift
   └─ Z.swift

instead of this:

S
├─ S.swift
├─ A
│  ├─ A.swift
│  ├─ A+X.swift
│  └─ Y
│     ├─ Y.swift
│     └─ Y+Equatable.swift
└─ B
   ├─ B.swift
   ├─ W
   │  ├─ W.swift
   │  └─ W+T.swift
   ├─ B+X.swift
   └─ B+Z.swift

Kyle-Ye · 2022-01-11T16:43:05Z

Sorry for reviving the discussion long after the PR is merged.

I find the prevalent use of "ParentType+" prefixes in file names makes the repository a little difficult to navigate. For example, in this screenshot of part of the repository structure in Xcode's sidebar:

Because of the shared prefixes, it becomes difficult to tell apart the files when their names are long (and the prefixes contribute to the file name lengths). This is can become much worse with types introduced in #12, which can lead to file names like SymbolGraph+SemanticVersion+Prerelease+Identifier.

I'd like to open a PR that amends the naming convention to the following, but would like to discuss it here first:
Keep the "+" convention for protocol conformances only, like in swift-collections.
Group files that define types under the same immediate parent types into the same directory with the file that defines the parent type, and do not repeat the parent type's name. <- It's a mouthful, but this is what I mean:
For a type structure like this:
struct S {
    struct A {
        struct X {}
        struct Y {}
    }
    struct B {
        struct W {
            struct T {}
        }
        struct X {}
        struct Z {}
    }
}

extension Y: Equatable {}
If we separate each type into its own file, then the directory structure should be like this:
S
├─ S.swift
├─ A
│  ├─ A.swift
│  ├─ X.swift
│  └─ Y
│     ├─ Y.swift
│     └─ Y+Equatable.swift
└─ B
   ├─ B.swift
   ├─ W
   │  ├─ W.swift
   │  └─ T.swift
   ├─ X.swift
   └─ Z.swift
instead of this:
S
├─ S.swift
├─ A
│  ├─ A.swift
│  ├─ A+X.swift
│  └─ Y
│     ├─ Y.swift
│     └─ Y+Equatable.swift
└─ B
   ├─ B.swift
   ├─ W
   │  ├─ W.swift
   │  └─ W+T.swift
   ├─ B+X.swift
   └─ B+Z.swift

Yes, both I and @franklinsch prefer the first approach I propose in the first post.

But this way we'll get more collision like Fragment+Kind.swift and Symbol+Kind.swift

Currently there are SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind and SymbolGraph.Symbol.Kind.

Although they are not in the same folder, they are in the same module. Which is not allowed by Swift

error: filename "Kind.swift" used twice: 'Symbol/DeclarationFragments/Fragment/Kind.swift' > and 'SymbolKit/SymbolGraph/Symbol/Kind.swift'
note: filenames are used to distinguish private declarations with the same name

belkadan.com/blog/2021/11/Swift-Mangling-Regret-Private-Discriminators

If we use the second approach (which is merged now), the "collision" only occurs once
and can use some strategy to work around.

Kyle-Ye · 2022-01-11T16:48:20Z

Because of the shared prefixes, it becomes difficult to tell apart the files when their names are long (and the prefixes contribute to the file name lengths). This is can become much worse with types introduced in #12, which can lead to file names like SymbolGraph+SemanticVersion+Prerelease+Identifier.

The file name will not be that long, it should at most be Prerelease+Identifier.swift according to the current rule.

WowbaggersLiquidLunch · 2022-01-11T17:00:24Z

The file name will not be that long, it should at most be Prerelease+Identifier.swift according to the current rule.

Yes, you're correct. I misinterpreted it. Although, I think my main point stands, that shared prefixes make it more difficult to discern files.

Kyle-Ye · 2022-01-11T17:09:56Z

I agree. So maybe we should only add the prefix when needed?
(Such as when a potential name collision may occur)

Kyle-Ye requested review from QuietMisdreavus and franklinsch December 11, 2021 09:00

Kyle-Ye force-pushed the split branch from b1fd16b to f95ab51 Compare December 13, 2021 16:52

Kyle-Ye force-pushed the split branch from f95ab51 to 133c548 Compare December 14, 2021 05:39

Kyle-Ye changed the title ~~[DNM & WIP] Split Symbol.swift to reduce complexity of the file~~ Split Symbol.swift to reduce complexity of the file Dec 14, 2021

Kyle-Ye force-pushed the split branch 2 times, most recently from 584b5cb to c0836b6 Compare December 14, 2021 05:58

Kyle-Ye force-pushed the split branch from c0836b6 to ecd7932 Compare December 15, 2021 16:18

Kyle-Ye mentioned this pull request Dec 15, 2021

change KindIdentifier into a struct to handle unknown symbol kinds #7

Open

QuietMisdreavus approved these changes Dec 15, 2021

View reviewed changes

Kyle-Ye added 2 commits December 16, 2021 12:42

Split Symbol.swift to reduce complexity of the file

a6312ef

Move Relationship.swift

f353774

Separate into 2 commits because git can't mark it as rename since the change is too big

Kyle-Ye force-pushed the split branch from ecd7932 to f353774 Compare December 16, 2021 04:43

Kyle-Ye requested a review from ethan-kusters December 16, 2021 16:05

ethan-kusters approved these changes Dec 18, 2021

View reviewed changes

Kyle-Ye merged commit c4c029c into swiftlang:main Dec 18, 2021

Kyle-Ye deleted the split branch December 18, 2021 02:54

Kyle-Ye restored the split branch December 18, 2021 02:55

Kyle-Ye mentioned this pull request Dec 18, 2021

[5.6] Cherry pick "Split Symbol.swift to reduce complexity of the file" #11

Merged

Kyle-Ye deleted the split branch December 21, 2021 04:41

WowbaggersLiquidLunch mentioned this pull request Jan 12, 2022

[NFC] Avoid shared prefixes in file names #14

Merged

Split Symbol.swift to reduce complexity of the file #8

Split Symbol.swift to reduce complexity of the file #8

Uh oh!

Conversation

Kyle-Ye commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Discussion

Status

Uh oh!

franklinsch commented Dec 13, 2021

Uh oh!

Kyle-Ye commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franklinsch commented Dec 13, 2021

Uh oh!

QuietMisdreavus commented Dec 13, 2021

Uh oh!

Kyle-Ye commented Dec 14, 2021

Uh oh!

Kyle-Ye commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kyle-Ye commented Dec 14, 2021

Uh oh!

Kyle-Ye commented Dec 14, 2021

Uh oh!

QuietMisdreavus commented Dec 14, 2021

Uh oh!

Kyle-Ye commented Dec 15, 2021

Uh oh!

QuietMisdreavus commented Dec 15, 2021

Uh oh!

Kyle-Ye commented Dec 15, 2021

Uh oh!

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Uh oh!

Kyle-Ye commented Dec 16, 2021

Uh oh!

Kyle-Ye commented Dec 16, 2021

Uh oh!

QuietMisdreavus commented Dec 16, 2021

Uh oh!

Kyle-Ye commented Dec 16, 2021

Uh oh!

Kyle-Ye commented Dec 16, 2021

Uh oh!

ethan-kusters left a comment

Choose a reason for hiding this comment

Uh oh!

ethan-kusters commented Dec 18, 2021

Uh oh!

WowbaggersLiquidLunch commented Jan 11, 2022

Uh oh!

Kyle-Ye commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kyle-Ye commented Jan 11, 2022

Uh oh!

WowbaggersLiquidLunch commented Jan 11, 2022

Uh oh!

Kyle-Ye commented Jan 11, 2022

Uh oh!

Uh oh!

Kyle-Ye commented Dec 11, 2021 •

edited

Loading

Kyle-Ye commented Dec 13, 2021 •

edited

Loading

Kyle-Ye commented Dec 14, 2021 •

edited

Loading

Kyle-Ye commented Jan 11, 2022 •

edited

Loading