Request for removing impl Module for Box<dyn Module>

[hyf0]

tl;dr

impl Module for Box<dyn Module> make &Box<dyn Module> look like a &dyn Module, but they are totally different things. People easily fall into traps.

details

I have seen some people debugging for not realizing &Box<dyn T> and &dyn T are different.

And that's the case I meet:

NormalModule overwrote the name_for_condition method of Module

rspack/crates/rspack_core/src/normal_module.rs

Lines 636 to 645 in 2c4c1da

	fn name_for_condition(&self) -> Option<Cow<str>> {
	// Align with https://github.com/webpack/webpack/blob/8241da7f1e75c5581ba535d127fa66aeb9eb2ac8/lib/NormalModule.js#L375
	let resource = self.resource_data.resource.as_str();
	let idx = resource.find('?');
	if let Some(idx) = idx {
	Some(resource[..idx].into())
	} else {
	Some(resource.into())
	}
	}

You put the NormalModule in the Box and erase the type Box and you get Box<dyn Module>.

Box<dyn Module> also implements the Module trait but forgets to redirect the name_for_condition method to the original NormalModule.

The re-direct code looks like

rspack/crates/rspack_core/src/module.rs

Lines 155 to 158 in 2c4c1da

	fn source_types(&self) -> &[SourceType] {
	(**self).source_types()
	}

The default implementation for name_for_condition method of Module is

rspack/crates/rspack_core/src/module.rs

Lines 93 to 97 in 2c4c1da

	/// Name matched against bundle-splitting conditions.
	fn name_for_condition(&self) -> Option<Cow<str>> {
	// Align with https://github.com/webpack/webpack/blob/8241da7f1e75c5581ba535d127fa66aeb9eb2ac8/lib/Module.js#L852
	None
	}

So, when you call name_for_condition() in &Box<dyn Module> you would always get a None.

traps

You will fall into traps:

• If you are adding a new method in Module but forgetting to redirect in impl Module for Box<dyn Module>
• If you forgot to redirect existing methods of Module in impl Module for Box<dyn Module>

solution

I have talked about this with @h-a-n-a before, the idea of impl Module for Box<dyn Module> is to reduce boilerplate code for deref Box.

I wonder whether there are other solutions to achieve the same goal. After investigating the code, I found most of the problems are due to using &BoxModule as params or return values.

Take this code as example

rspack/crates/rspack_core/src/module_graph/mod.rs

Lines 162 to 165 in 2c4c1da

	#[inline]
	pub fn module_by_identifier(&self, identifier: &ModuleIdentifier) -> Option<&BoxModule> {
	self.module_identifier_to_module.get(identifier)
	}

There's no need for returning Box<&dyn Module> due to no use cases(so far) caring about the Box in Box<&dyn Module>. Returning Option<&dyn Module> would be enough.

The argument also applies to using &BoxModule in function parameters. So, we just need to replace all &BoxModule with &dyn Module, which would also achieve the goal: reduce boilerplate code for deref Box and avoid the traps mentioned above.

[IWANABETHATGUY]

For this question:

What about removing the default implementation?

[hyf0]

If that's the case we need to remove all default implementations to get checking from the compiler.

[hyf0]

sync up:

We would remove impl Module for Box<dyn Module> in the future but I don't have time now. I would create a good first issue
in case anyone wants to do this.