chore(transport): remove module_name_repetitions

[mxinden]

We allow the clippy lint module_name_repetitions:

neqo/neqo-transport/src/lib.rs

Line 7 in a758177

#![allow(clippy::module_name_repetitions)] // This lint doesn't work here.

We e.g. prefix various types in the ecn module with Ecn:

neqo/neqo-transport/src/ecn.rs

Lines 18 to 30 in 3001a3a

	/// The number of packets to use for testing a path for ECN capability.
	pub const ECN_TEST_COUNT: usize = 10;
	/// The number of packets to use for testing a path for ECN capability when exchanging
	/// Initials during the handshake. This is a lower number than [`ECN_TEST_COUNT`] to avoid
	/// unnecessarily delaying the handshake; we would otherwise double the PTO [`ECN_TEST_COUNT`]
	/// times.
	const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3;
	/// The state information related to testing a path for ECN capability.
	/// See RFC9000, Appendix A.4.
	#[derive(Debug, PartialEq, Clone, Copy)]
	enum EcnValidationState {

As discussed in #2270 (comment), I suggest removing the allow and instead fix the module name repetition.

[omansfeld]

I'll start working on this! Are there any places where we don't want to get rid of the module name repetitions? As far as I can see we allow them in a lot of modules, just going by this quick search.

Maybe it makes sense to remove the crate-wide allow attributes (as the one referenced above) and leave be or possibly add local allow's where it makes sense to keep the module name repetitions?

[mxinden]

@mansf-osk I suggest starting with a single module, e.g. neqo-transport/src/ecn.rs.

Small pull requests reduce the likelihood of merge conflicts and make reviewing easier.

Maybe it makes sense to remove the crate-wide allow attributes (as the one referenced above) and leave be or possibly add local allow's where it makes sense to keep the module name repetitions?

Happy for the crate-wide #![allow(clippy::module_name_repetitions)] to be replaced by the missing local allows with the first pull request.

[larseggert]

Just so I understand, we would rename e.g. EcnInfo to Ecn::Info, i.e. prefix with the module name? (Because otherwise Info is a bit generic.)

[mxinden]

It would be defined as:

// neqo-transport/src/ecn.rs

pub struct Info {
  // ...
}

and it would be used as:

// Some other file.

use ecn;

fn my_func() {
  let ecn_info = ecn::Info::new();
}

Just like in the standard library where it is std::io::Error and not std::io::IoError.

Does that make sense @larseggert?

[omansfeld]

I'd like some feedback on the following before eventually tackling more of this refactor!

The way we did it in #2311 (like in Max's example above) we would eventually end up losing lots of explicit imports that we have right now. I'm not that big a fan of that personally, so I'd like to use aliases instead, as is also suggested in the discussions around that lint's RFC. I especially like the following sentiment in that comment:

"Essentially, this pushes resolving overlaps between names to the client of an API (where the overlaps are actually created) rather than the provider of an API (who would otherwise have to guess at which overlaps will occur)."

Going by the example above it would look like that:

use ecn::{Info as EcnInfo, Config as EcnConfig}

fn my_func() {
    let ecn_info = EcnInfo::new();
    let ecn_config = EcnConfig::new();
    // ...
}

That way we still have the prefix for clarity where we consume the API while also adhering to the lint and not losing all our explicit imports. As I see it this is the most canonical way to implement that lint.

Thoughts?

[larseggert]

WFM, but it requires the importer to not be lazy and leave the import named as-is. If the importer was lazy, your example would look like this, which I think would be bad:

use ecn::{Info, Config}

fn my_func() {
    let ecn_info = Info::new();
    let ecn_config = Config::new();
    // ...
}

[mxinden]

The way we did it in #2311 (like in Max's example above) we would eventually end up losing lots of explicit imports that we have right now. I'm not that big a fan of that personally, so I'd like to use aliases instead, as is also suggested in the discussions around that lint's RFC.

I don't think "losing lots of explicit imports" is a problem.

In my eyes importing a module (e.g. use ecn;) only is simpler,

use ecn;

fn my_func() {
  let ecn_info = ecn::Info::new();
  let ecn_config = ecn::Config::new();
}

than the aliasing:

use ecn::{Info as EcnInfo, Config as EcnConfig}

fn my_func() {
    let ecn_info = EcnInfo::new();
    let ecn_config = EcnConfig::new();
}

[omansfeld]

Thanks for the feedback, I'll keep it as is then. Will put the refactor in my backlog and chip away at it when I have bandwidth.

[larseggert]

OBE? I don't see module_name_repetitions in the codebase anymore.

[omansfeld]

It looks like all the allow's were removed in #2456 so now cargo clippy spits out loads of warnings.

[larseggert]

It doesn't for me? How do you run it?

[omansfeld]

I just literally run barebone cargo clippy on the latest commit and there are 93 mentions of module_name_repetitions in the output.

[larseggert]

What toolchain version are you on?

[larseggert]

Ah, I see it with 1.82. We only run clippy on stable in CI, which explains why it doesn't fail there.

[omansfeld]

Yes, I was still on 1.83.0 and now used that opportunity to update. Don't see it on 1.86.0 anymore.

I'm curious in case you know, what was changed between toolchain versions that this now isn't an issue anymore? What was the reason for deleting all the allow's and why are the module_name_repetitions warnings not firing anymore with them deleted?

Do we still want to do this long-term chore, or should the issue be closed?

[omansfeld]

Ah, I see, module_name_repetitions were moved from pedantic to restriction in rust 1.84.0.

[larseggert]

We should probably check it we want to turn on any of the lints in https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow&groups=restriction

[omansfeld]

In my opinion the argument for removing the module_name_repetitions still stands, no matter if the clippy team moves it into restriction or pedantic. I'm for enabling it again so we are reminded to write new code accordingly and can do existing renames when code is touched/we want to.

If there is consensus for it I'll go ahead and turn module_name_repetitions on again.

[larseggert]

Go for it.

[mxinden]

If there is consensus for it I'll go ahead and turn module_name_repetitions on again.

Sounds good to me.