feat: check duplicate dependencies

[orhun]

fixes #715

e.g. if you add:

[dependencies]
python = "3.11.*"
flask = "2.*"
Flask = "2.*"

pixi run fails as:

  × failed to parse pixi.toml from /home/orhun/gh/pixi/examples/flask-hello-world
  ╰─▶ duplicate dependency: Flask

[baszalmstra]

Im wondering if it would make more sense to add a custom deserializer implementation for this (using serde_with) instead. Because that would allow you to error out while deserializing the fields which provides in context error messages.

[orhun]

I'm not sure if it is possible to collect different fields (run, build, host dependencies) and check for duplicates in them 🤔

[ruben-arts]

Could just throw an error if capitalization is used right?

[baszalmstra]

I'm not sure if it is possible to collect different fields (run, build, host dependencies) and check for duplicates in them 🤔

In that case I think its fine, because these can overwrite each other. This is OK (although weird):

[dependencies]
foobar = 1

[build-dependencies]
foobar = 2

This is not OK:

[dependencies]
foobar = 1
Foobar = 2

[baszalmstra]

Could just throw an error if capitalization is used right?

Thats an alternative approach, to only allow normalized names in the pixi.toml.

[orhun]

I added a function for deserializing dependencies in a1df903

I can add tests if you think it looks good.

[ruben-arts]

I think it is best to removed to chaining of the different dependency fields. As I just understood bas' comment. We broke the functionality with this changed.

My plan was to error on:

[dependencies]
foobar = "*"

[build-dependencies]
FooBar = "*"

But we now also error, but shouldn't on:

[dependencies]
foobar = "*"

[build-dependencies]
foobar = "*"

The plan to avoid any capitalization is still a thought but might be breaking. Lets throw it in the community.

[orhun]

I see, so just checking the uppercase for each dependency section is enough?

[baszalmstra]

No its not. Especially Pypi dependencies have more rules than just lowercasing (dashes, dots and underscores).

Would this work?

#[serde(default)]
#[serde_as(as = "IndexMap<DisplayFromStr, PickFirst<(DisplayFromStr, _)>>")]
dependencies: IndexMap<NormalizedPackageName, NamelessMatchSpec>,

[orhun]

Not really.

NormalizedPackageName is deserialized from the FromStr implementation (via DisplayFromStr):

impl FromStr for NormalizedPackageName {
    type Err = ParsePackageNameError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        Ok(PackageName::from_str(s)?.into())
    }
}

And it is basically the same thing as PackageName.

[baszalmstra]

A maybe thats something we can change! We could implement a custom DeserializeAs (StrictNormalizedName) or whatever, that requires the normalized name to be the same as the non-normalized name?

[orhun]

Thanks for the tip @baszalmstra!

I went with a waaaay simpler approach in c70586c

[baszalmstra]

I hate to say this but I'm still not entirely sure about this approach.

The downside of this approach is that existing pixi.tomls that contain non-normalized names will break. We know that there are examples of this (#264).

I see two routes that we can take:

1. Instead of erroring, raise a warning about this problem and that in the future this will be an error.
2. Write a custom Deserializer that while it builds the mapping also checks each key of the map for duplicates. Im confident that with some thinkering we can build this in such a way that we can both allow non-normalized names but check for duplicates and have contextual errors. Probably using some custom DeserializeSeed implementations.

Id prefer we take the second route. If you don't feel as confident as I in the second route I'd be happy to take a stab at it.

[orhun]

That's totally reasonable. I'm happy to take the second route and ping you in case I need any guidance 🐻

[orhun]

@baszalmstra I took a look at this again and there are a couple of points that I want to clarify:

• Write a custom Deserializer that while it builds the mapping also checks each key of the map for duplicates.

I came up with this:

fn deserialize_package_map<'de, D>(
    deserializer: D,
) -> Result<IndexMap<PackageName, NamelessMatchSpec>, D::Error>
where
    D: Deserializer<'de>,
{
    struct PackageMapVisitor(PhantomData<()>);

    impl<'de> Visitor<'de> for PackageMapVisitor {
        type Value = IndexMap<PackageName, NamelessMatchSpec>;

        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
            write!(formatter, "a map")
        }

        fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
        where
            A: MapAccess<'de>,
        {
            let mut result = IndexMap::new();
            while let Some((k, v)) = map.next_entry::<String, String>()? {
                let package_name = PackageName::from_str(&k).map_err(serde::de::Error::custom)?;
                let match_spec =
                    NamelessMatchSpec::from_str(&v).map_err(serde::de::Error::custom)?;
                if result.insert(package_name, match_spec).is_some() {
                    return  Err(serde::de::Error::custom(
                        "invalid dependency: please avoid using capitalized names for the dependencies",
                    ));
                }
            }

            Ok(result)
        }
    }
    let visitor = PackageMapVisitor(PhantomData);
    deserializer.deserialize_seq(visitor)
}

However, this is used with serde(with) attribute which means we can no longer use serde_as:

- #[serde_as(as = "IndexMap<_, PickFirst<(DisplayFromStr, _)>>")]
+ #[serde(default, deserialize_with = "deserialize_package_map")]
dependencies: IndexMap<PackageName, NamelessMatchSpec>,

Also, in the case of Option<IndexMap<...>, this won't work.

• Both allow non-normalized names but check for duplicates and have contextual errors

Right now we get:

  × failed to parse pixi.toml from /home/orhun/gh/pixi/etc/issue717
  ╰─▶ failed to parse project manifest
    ╭─[pixi.toml:11:1]
 11 │
 12 │ ╭─▶ [dependencies]
 13 │ │   python = "3.11.*"
 14 │ │   flask = "2.*"
 15 │ ├─▶ Flask = "2.*"
    · ╰──── invalid dependency: please avoid using capitalized names for the dependencies
    ╰────

What do you mean by allowing non-normalized names? Isn't this a contextual error?

• Probably using some custom DeserializeSeed implementations.

I took a look at the DeserializeSeed example and couldn't wrap my head around how that can be applied to this case. If I understood correctly, it is used when there is an already allocated type that we want to pass into a Deserializer. In our case we can probably allocate the map inside the Deserializer as well.

Maybe I got it wrong though. Do we have any usages of DeserializeSeed already that I can use as an example?

[baszalmstra]

@orhun that's indeed what I had in mind. We indeed lose serde_as which is fine I think.

Instead of deserializing the key value pairs as Strings you could directly parse them as PackageName and NamelessMatchSpec (or some wrapper type to allow custom deserializers). The NamelessMatchSpec can be deserialized using serde_untagged instead of using serde_with (its either a string or the mapped object itself).

The only remaining issue is that now any error is emitted at the level of the entire dependencies block. I think there you could use a DeserializeSeed implementation for the package name which you pass a mutable reference (or some *Cell type) to the map in construction. The deserializer impl then checks if an entry with the same key already exists and errors there. That will then raise the error for the key itself instead of for the containing block. Hopefully, that yields error Valhalla bliss!

[orhun]

Made the requested changes in ea724bf

2 things:

• I added deserialize_package_map and deserialize_opt_package_map for deserializing Option<T> and T - not sure if there is a cleaner way of doing this.
• How should we test this? Is adding unit tests for deserialization enough?

[baszalmstra]

@ruben-arts Please test to see if you liky!

[ruben-arts]

First thing I tried was this:

😅

I do see that as an issue, should we make an issue for it to merge this or do you think it is easy to fix with this PR. I'm fine either way as it already is an improvement over the current state.

[baszalmstra]

What do you mean? The error indicates there is a duplicate flask entry which is indeed the problem?

[ruben-arts]

What do you mean? The error indicates there is a duplicate flask entry which is indeed the problem?

The add still lets you add a duplicate, leaving the toml in a broken state. I think we should never do that.

[baszalmstra]

Aaah! Yeah now I get it! That makes sense.

[orhun]

Should we also check duplicates for the pixi add command? Maybe that can be handled in a separate PR.

[ruben-arts]

I would ask you to start on that immediately but we can merge this for now.