loading code simplifications

[vtjnash]

In my attempts to better understand the existing loading code (before making more major changes, such as making the environment maps explicit for the Distributed and precompile code), I experimented with trying to simplify the code logic here and adding or expanding upon comments. I turned up a few minor bugs (such as places where we returned nothing::Bool) and other slight inconsistencies (such as potentially attempting to search X/src/X.jl/src/X.jl). The biggest change here is removing all of the 3VL (UUID, true, or false) in favor of nullable (PkgId or nothing).

Relatedly, I then turned to the code-loading docs, and attempted to simplify and clarify those, and correct a couple places where the code doesn't quite do what was documented. I'd like to do a bigger rearrangement of this also (splitting it into a couple files, and possibly adding more support to Documenter to handle nested documents better), but sometime later.

I'm not strongly opinionated about merging all of this, and rather expect that we might cherry-pick some bits or alter others further (both in the docs changes and in the code).

[StefanKarpinski]

I'm not entirely sure I would call a 40 line increase of code a simplification. I'll review in more detail, but in terms of simplicity this seems like pretty much a wash to me.

[StefanKarpinski]

Since the meat of this change is switching from Union{Bool,UUID} in several places in the API to Union{Nothing,PkgId}; so can you give a little motivation for this change? I'm not objecting to it, but I'm not entirely convinced that it's uniformly better, but would like to be convinced.

[StefanKarpinski]

One simplification that I had been wanting to do was to use the nil UUID uniformly to represent the absence of a UUID instead of the Union{Nothing,UUID} business. This would simplify things since we do that in the serialized module anyway. If the rest of the code understood the nil UUID to mean "no UUID" it would probably make things smoother all around.

[vtjnash]

I'm not entirely sure I would call a 40 line increase of code a simplification

I don't think that SLOC has much association to code-complexity. I find it's nice when they happen to correspond, but that generally longer code can be simpler and therefore clearer. For example, the old code had a function named (approximately) project_file_name_&_uuid_&_path. This saved a few lines over having separate helper functions for each item (project_file_id and project_file_path), but makes the code confusing and unclear.

can you give a little motivation for this change

Yes. As I mentioned above "the biggest change here is removing all of the 3VL (UUID, true, or false) in favor of nullable (PkgId or nothing)." (Actually, my motivation for changing it was as a code-exercise to test my understanding of it and look for any bugs or unexpected interactions that I should be aware of. As such, I tried to change or re-write anything that seemed overly-complex—and see if that helped clarify it.)

[StefanKarpinski]

"the biggest change here is removing all of the 3VL (UUID, true, or false) in favor of nullable (PkgId or nothing)."

Ok, but why is that better? Can you give some motivation?

[vtjnash]

OK, I get that that's not directly a reason, but merely seemed to be evidence of there being an issue. But explaining why it felt bad is harder. It just felt wrong to me, because it didn't quite match the documentation. Each of these functions was implemented to return UUID|true if the result is a PkgId, else return false if the result is not-found/nothing. Then the caller was expected to decode the result. That's doable—and I can now reimplement the few minor bugfixes here for that encoding. What this PR does instead is to remove that intermediate encoding, and like the docs, just work always in terms of whether the package is identified or not.

[StefanKarpinski]

Then the caller was expected to decode the result. [...] What this PR does instead is to remove that intermediate encoding

Ok, that makes sense. Thanks for explaining the motivation. I'll take a look in light of that.

[vtjnash]

Given the impending code freeze, I'd like to merge this in a couple days.

[StefanKarpinski]

I suppose that's fair given that I haven't reviewed it yet, but I'd still like to review it, so I guess that puts a deadline on my review.

[StefanKarpinski]

You don't want to call realpath on a project_file that doesn't exist since it breaks. Perhaps you've rejiggered the code so that never happens but it seems better to either return nothing or perhaps to return the nil UUID since that's what we say the UUID of a non-existing project file is?

[vtjnash]

It couldn't happen before either (we only call it immediately after calling open(project_file), and that certainly would have failed if it didn't exist). I assume at some point it was possible, but then got refactored out by the time the PR was made.

[StefanKarpinski]

It took me quite a while to review #29946, so I've barely started reviewing this. I can take a look at it again next week, please don't merge until I've gotten to review.

[vtjnash]

Bump. It's unfortunate this has gotten lost from v1.1

[StefanKarpinski]

I'm still not convinced that this is actually a simplification—it is a net gain of 50 lines of code, after all. But I guess it's probably not harmful as long as the very thorough test suite continues to pass.