Rename Registry trait

[Ericson2314]

AFIAK, the terminology we want is:

• a source is a play to find packages (specifically their source code)
• a registry is a type of source which contains multiple released packages (not WIP packages -> it's not a vcs)

But right the Registry trait is a super-trait of Source. At the very least Registry should get a new name, but perhaps both traits should be renamed like RemoteSource: Source or CachedSource: Source.

[dwijnand]

Looking at the code to try and derive inferences I came up with the following notes:

// Desired:
// * a source is a place to find packages (specifically their source code)
// * a registry is a type of source which contains multiple released packages (not WIP packages)
//
// "perhaps both traits should be renamed to `RemoteSource: Source` or `CachedSource: Source`"

/// The source of information about a group of packages
trait Registry {}

/// A Source finds and downloads remote packages based on names and versions.
trait Source: Registry {}

// The following have both Registry and Source impls:
struct DirectorySource {}
struct       GitSource {}
struct      PathSource {}
struct  RegistrySource {}
struct  ReplacedSource {}

/// A registry of known packages.
/// Contains a collection of `Source`s, which are used to load a `Package` from.
struct PackageRegistry  {}
impl Registry for PackageRegistry {}

Note PackageRegistry is the only struct that is just a Registry but not a Source.

I think the "registry" referred to above only applies to RegistrySource; for the other Registry+Source structs that description I think doesn't match.

Is the parallel of Registry vs Source: apt-cache vs apt-get, maybe?

I've been thinking of names like "QuerySource", given "query" is effectively the defining method of Registry.

Thoughts?

[matklad]

Note PackageRegistry is the only struct that is just a Registry but not a Source.

Excellent! I think this is a core observation here! Note that we are not really using PackageRegistry generically: that is, everything would almost work, if PacakgeRegistry did not implement Registry, and instead had the query inherent method. "almost" because we do substitute it with another implementation for some tests.

And that probably means that Source and Registry should not inherit from each other at all. Let's try to remove the inheritance before any renames?

I think the path would be

• move supports_checksums and requires_precise methods from Registry directly to Source. I am pretty sure that this would work
• add query method to Source, and remove : Registry bound. I think this would work, and, despite the seeming duplication, it should reduce the net amount of code.

[dwijnand]

With #5476 is renaming still of interest? Or can it be said this issue is "resolved" with #5476?

[Ericson2314]

Err so now neither is a supertrait of the other? That gives 2^2 = 4 combinations, are all 4 needed?

[matklad]

Err so now neither is a supertrait of the other? That gives 2^2 = 4 combinations, are all 4 needed?

They are disjoint now: each type is either only a source, or only a registry.

I still think that rename is needed, because the term Registry is overloaded.

First of all, we have sources. a Source is a thing with identity (SourceId), which holds a bunch of packages. One package for path sources, several for git source, and lots for registry source. In this sense, registry is kind of source, which holds a lot of packages, as well as a database of available pacakges, in crates.io index format.

During resolution, Cargo works with packages from different sources, Registry the trait just bundles all those sources in a single struct, it does not have an identity. So a possible name for Registry trait would be SourceSet. However, I don't have a nice suggestion for PackageRegistry (which is the single non-test-mock implementation of SourceSet).

Also, we have a thing called RegistryQuerier, which wraps PackageRegistry, and looks like they do more or less similar stuff: wrapping a bunch of Sources and adding stuff like overrides and replacements.

[dwijnand]

From #5432, I'm going to assume @alexcrichton might have an opinion about "naming things". 😉

[Ericson2314]

@matklad

I still think that rename is needed, because the term Registry is overloaded.

Yes at the very least let's do that. Otherwise it's hard to even discuss the issues.

They are disjoint now

Err a type can implement both or neither in principle.

In this sense, registry is kind of source, which holds a lot of packages, as well as a database of available pacakges, in crates.io index format.

This would be the registry type, right?

Registry the trait just bundles all those sources in a single struct, it does not have an identity.

I'd make a trait with all the methods, and a sub trait with just the identity part. A set of sources is also a source, but a set of "identified sources" doesn't itself have an identity.

Right now we have identical methods in both traits which seems funny :).

[dwijnand]

Wouldn't it be better to just remove the indirection of Registry and use PackageRegistry directly?

I tried to go about that, but ran into lifetime issues (see #5476 (comment)).

[Ericson2314]

@dwijnand I saw you tried to do that, and it seems reasonable, but what about the test that mocks it up? Also, I like the idea that sources (sans identification) compose. To me, composition like that is just a canary for a good design, and should be pursued even absent a concrete use-case.

[dwijnand]

but what about the test that mocks it up?

No idea, I didn't get to it as I got stuck trying to check the borrowing in the main code. 🙂

But (hand wavey) if PackageRegistry is a set of sources my hope was the tests could use real PackageRegistry with a singleton set of a replacement source.

[Ericson2314]

@dwijnand I guess think about this way: even if we did get rid of the trait, I rather PackageRegistry implement source rather than duplicate the inherit method's type, as duplication is always suspicious. If that means making two traits as PackageRegistry cannot implement all the methods, so be it. So worse-case we delete one trait and make another, rather than reusing the Registry trait.

[dwijnand]

I'm not that fussed, either way. If a consensus is reached and the work hasn't been done, I'm happy to (attempt to) do the grunt work.

[Ericson2314]

Sounds good. Thanks for getting the ball rolling :).

[matklad]

I tried to go about that, but ran into lifetime issues

The "lifetime issues" are so interesting, that I've even blogged about them! :) The TL;DR is that using a trait object here indeed simplifies lifetime management. This, and the presence of the mock suggests that we probably shouldn't try to replace registry trait object with a concrete struct.

Right now we have identical methods in both traits which seems funny :).

@Ericson2314 the fact that methods have the same signature and name does not mean that their semantics is also identical :) It is wrong to pass bare sources to various resolve-related method. This has an interesting practical implication as well. Previously, one could see that resolve operates on &Registry trait object, use "navigate to implementation" IDE shortcut to find out what that Registry could be, and find a dosen of different impls. Now, there are only two impls, one of which is a test double, so it becomes immediately clear, statically, which code is executed when resolve calls registry methods.

So, my opinion here:

• Source and RegistrySource names are good.
• trait Registry, struct PackageRegistry and struct RegistryQueryer names are confusing, because they are not registries, but a collection of arbitrary sources, some of which might be registries.
• we probably should not get rid of trait Registry, b/c it simplifies lifetimes and testing.

So, we should just rename the three "Registry"s. I personally can't think of a good name for them though :( Something along the lines of SourceSet or MetaRegistry might work, but we need three of them 😨

[Ericson2314]

the fact that methods have the same signature and name does not mean that their semantics is also identical :)

I'm...glad you have the ":)".

It is wrong to pass bare sources to various resolve-related method.

Why? Does it need to be wrong?

:)