SR-12247 : Replace the type alias ModuleDecl::ImportedModule with a dedicated struct

[dfsweeney]

This is for SR-12247: Replace the type alias ModuleDecl::ImportedModule with a dedicated struct. It creates a new struct ImportedModule to replace the earlier std::pair implementation. It runs through the client code and replaces .first and .second with .accessPathTy and .moduleDecl.

The two-argument constructor for ModuleDecl::ImportedModule will fail with an assertion if the .moduleDecl pointer is null. The zero-argument constructor puts a nullPtr in .moduleDecl. There are cases in client code, for instance lib/Serialization/ModuleFile.h:113:32: ModuleDecl::ImportedModule Import = {}; where the zero-argument constructor is called. I am not completely sure, but I think the initialization case is the only time the zero-argument constructor is called. It might be possible to replace that with a singleton. The field in the example is replaced completely later.

It's not possible right now to make the .accessPathTy and .moduleDecl struct elements const because there are some calls where the struct is modified after creation. It might be possible to clear this up--it needs more work. The ImportedModules go through some std:: data structures and some of them expect to be able to mutate them, but may not actually need to.

@varungandhi-apple added the issue to bugs.swift.org.

Resolves SR-12247.

[varungandhi-apple]

Thanks for creating the PR!

@swift-ci please test

[varungandhi-apple]

For making changes, I suggest adding more commits instead of force-pushing (unless you have a strong preference for force-pushing 🙂). In the end, after tentative approval, you can squash them for a clean history (since this change doesn't need more than 1 commit, I think?), we can re-run the tests and then merge.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - d1889d83bf2ce645f8d7f4a9fcb3eb71a25805f8

[varungandhi-apple]

The two-argument constructor for ModuleDecl::ImportedModule will fail with an assertion if the .moduleDecl pointer is null.

Is this constructor being called anywhere? The places I'm looking at are all using construction from an initializer-list -- maybe my C++-fu is not up to scratch but I think that will call an auto-generated constructor, not the constructors you wrote down manually... am I misunderstanding something?

There are cases in client code, for instance lib/Serialization/ModuleFile.h:113:32: ModuleDecl::ImportedModule Import = {}; where the zero-argument constructor is called.

Hmm. Maybe that field should be an Optional<ModuleDecl::ImportedModule>? You could leave a TODO: Should this field have type Optional<ModuleDecl::ImportedModule>? there (someone can fix that in a later PR).

I am not completely sure, but I think the initialization case is the only time the zero-argument constructor is called. It might be possible to replace that with a singleton. The field in the example is replaced completely later.

I think it's ok to leave it as is for now (also, since it is essentially a wrapper over a pair of pointers and a length, a singleton object wouldn't really help.)

[varungandhi-apple]

It's not possible right now to make the .accessPathTy and .moduleDecl struct elements const because there are some calls where the struct is modified after creation. It might be possible to clear this up--it needs more work.

I would not worry about this for, it's not a big deal. We do need the ability to update the fields, I don't think there's an easy way to avoid that here.

[varungandhi-apple]

Hmm, looks like the CI is failing when compiling SourceKit, with lines like:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/tools/SourceKit/lib/SwiftLang/CodeCompletionOrganizer.cpp:386:36: error: no member named 'second' in 'swift::ModuleDecl::_ImportedModule'
      worklist.emplace_back(import.second, next);
                            ~~~~~~ ^

You'll need to update SourceKit too. 😅 The usual build-script invocation should build SourceKit as well. If you want to rebuild using ninja, you can do xcrun ninja (without any arguments) in the build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64 (or equivalent) directory, that will incrementally rebuild the compiler, SourceKit, the stdlib and other tools.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - d1889d83bf2ce645f8d7f4a9fcb3eb71a25805f8

[dfsweeney]

Thanks for looking at this! I should have done another cleanup pass through this before I created the pull request. I will work through these this morning.

[varungandhi-apple]

I should have done another cleanup pass through this before I created the pull request.

It's ok. When I'm working on multiple things, I sometimes forget to clean up one or the other before submitting a PR. No big deal.

[dfsweeney]

I rebuilt with 'ninja' instead of 'ninja swift' and it rebuilt SourceKit, which caught the ones I missed.

WRT the constructor question, I have to think about that more. I believe that when initializing the constructors are called if they are explicitly set. I am basing that belief on the behavior of the compiler based on some of the variations of code I tried. I am not sure I am right about that though and should recheck it.

(My quick reskim of C++ Crash Course implies that if you use a braced initializer the compiler will substitute a call to the constructor with the corresponding arguments if it exists, but I might still be inferring too much there. This gets kind of subtle, between the aggregate initializations and so on. Still working on it.)

It might be good to make the {} initializer an Optional for a lot of reasons. I only saw one instance of that in the code but that might have been the first one that the compiler saw. I will look further.

[dfsweeney]

I don't think I can do this but here goes :) :

@swift-ci please test

[dfsweeney]

A little more on the constructor thing--if I comment out the zero-argument constructor the compile fails for situations where you have something like

ImportedModule x = {};

I think there is a zero-arg constructor for std::pair that it was catching before.

[varungandhi-apple]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - d1889d83bf2ce645f8d7f4a9fcb3eb71a25805f8

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - d1889d83bf2ce645f8d7f4a9fcb3eb71a25805f8

[varungandhi-apple]

Also, small thing but the #1156 that you wrote in the comment is linking to Swift's #1156, not LLDB's. 😅 To do that, you can copy the URL directly swiftlang/llvm-project#1156

[dfsweeney]

Yeah I agree. In the am I will try to fix it.

…

Sent from my iPad

On Apr 29, 2020, at 23:03, Varun Gandhi ***@***.***> wrote:  Something has gone wrong here with the rebase; I don't get why there is a merge at the end. After the rebase, I would've expected this PR would only have 7 commits all of which are by you. Instead, there are 128 commits in this PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[dfsweeney]

Sorry! I'm not sure why my rebase went so awry. I think I fixed it but want to build locally before I push the branch up. Is there something I can do to help or not harm more?

[varungandhi-apple]

This broke the build. Does the pre-merge smoke test build this? If so, why did this make it past it? If not, can the smoke test be changed to build docs?

This hasn't been merged yet, so it cannot break the build (usually when we say "broke the build", we mean that master/a release branch itself is failing to build or failing some tests, which is not the case here)... What might have happened is that some PRs got batched along with this PR and that might have caused a failure, that happens for PRs sometimes. To work around that, you can do a clean smoke test or a clean test.

[dfsweeney]

@varungandhi-apple I think the rebase problem is because I have origin pointing to apple/swift and a 'dfsweeney' remote pointing to dfsweeney/swift, and the dfsweeney/master was behind the origin/master, and I used the dfsweeney remote in the wrong place. Or possibly vice versa for some combination of vice and versa. Sorry about that. Something like this happened to me before, so I think I don't understand which remote to use when. I set up a new branch and cherry-picked the good commits on top of it. I am building it now and will force-push if it builds and tests.

[dfsweeney]

@martinboehme Re the build--I noticed while in the middle of this that the documentation step was failing my local build. I did not touch the documentation but it was odd. It was saying that 'associated type' was not in the glossary, I believe.

[varungandhi-apple]

It's ok, things sometimes go wrong. Here's one way to go about fixing it.

1. git branch -m sr12247 sr12247-old: rename your existing branch locally.
2. git checkout master.
3. git remote -v: double-check the origin remote points to apple/swift.
4. utils/update-checkout --scheme master: Double-check that this succeeds.
5. git checkout -b sr12247
6. git cherry-pick bbe2f6e735e98c7b1d21aa91b804705e750a532f .. 83bdf84f261f1f3defa54dd9d3568169e5ebd6cd (I've taken the commit immediately before your first commit, because IIRC cherry-pick takes the start commit exclusive and the end commit inclusive): Resolve any conflicts while cherry-picking (IIRC this involves git add after fixing the conflict and git cherry-pick --continue) that arise and double-check that that you got all your 6 commits cherry-picked successfully.
7. git push --set-upstream dfsweeney sr12247 --force-with-lease: Double-check that this succeeded in the GUI.
8. @ me here, so we can kick off the paired tests.

Also, you should always feel free to ask for help with git. 🙂 The UX of git is pretty terrible, so it is easy to muck things up.

[dfsweeney]

@varungandhi-apple I had got up to 7. on your list this morning, but I forgot that you need to go one commit back, so I got a bunch of merge conflicts, which I fixed, before, whatever. I guess it's good to look at your code closely.

It was much faster when I redid it including the commit immediately before the first one.

I did the force-with-lease push and it looks ok now on the GitHub web side it says 7 commits now. I am running the build and tests locally here now but it will take a bit because it needs to rebuild everything again. This has been an exciting morning.

@varungandhi-apple Thank you for your help!

[varungandhi-apple]

Huh that's weird, I'm pretty sure I counted 6 commits, not sure which one I missed. Anyways, doesn't matter, since we will squash anyways later.

@swift-ci please test
swiftlang/llvm-project#1156

[dfsweeney]

For some reason void swift::diagnoseAttrsRequiringFoundation(SourceFile &SF) in Sema/TypeCheckDeclObjc.cpp is still in there at the tip of the branch when it should not be (Hamish Knight took it out a couple of days ago in unrelated work but I edited it). I think the swift build will fail on that. There might still be something off about the commit history here.

[varungandhi-apple]

Good catch, maybe the conflict didn't get resolved correctly? Please do remove that function 🙂. While you're at it, there's a couple of stray whitespace changes in Module.h, you could remove those as well. After that, you could also squash the changes (use git rebase -i commit-hash-before-your-first-commit mark all but your first commit as squash instead of pick, although I don't recall if the commit message would get messed up, you should double-check that), since we're not expecting more changes here.

[dfsweeney]

Got it. Once I have squashed, should I git push --force to fix it up? After the local rebase I have

Your branch and 'dfsweeney/sr12247' have diverged, and have 1 and 7 different commits each, respectively. (use "git pull" to merge the remote branch into yours)

I think that git pull advice is where I went wrong before so I am double-checking 😀

[varungandhi-apple]

Yeah, git push --force should do it. (Also, that's kinda why I suggested renaming your old branch to something that's still available. In case the cherry-pick/rebase/force push gets messed up, you can go back to your old branch and have that state of progress, instead of trying to rummage through the output of git reflog.)

[varungandhi-apple]

@swift-ci please test
swiftlang/llvm-project#1156

[varungandhi-apple]

Why are the tests not working? 🤔

swiftlang/llvm-project#1156
@swift-ci please test

[dfsweeney]

I see them now. now I see the one that failed and the next one :)

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 1d625d819ed56468d2c45d44d5bfeecd18a94ed4

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 1d625d819ed56468d2c45d44d5bfeecd18a94ed4

[dfsweeney]

It looks like there was an infrastructure failure installing llbuild in Swift Test OS X Platform. I was keeping an eye on it in the background--it got pretty far, through building swift and lldb and testing swift and lldb. I was hoping it was coming in for a landing soon.

[theblixguy]

swiftlang/llvm-project#1156
@swift-ci please test macOS

[varungandhi-apple]

Tests were passing, so I've merged the changes! Thank you for your patience in responding to all the review comments! 😄

[dfsweeney]

Thanks for reviewing! This is really exciting for me. It is good to know I can focus enough to do this during everything else that’s happening. Thanks for your help, Dan

…

On Apr 30, 2020, at 9:27 PM, Varun Gandhi ***@***.***> wrote: Tests were passing, so I've merged the changes! Thank you for your patience in responding to all the review comments! 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31360 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAWZBH2MJR56PZQQY3ISGLRPI6RVANCNFSM4MSO4PGQ>.