refactor: DerefOpiton default to Some(T) when T impl Default trait #12406
Conversation
✅ Deploy Preview for rspack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
team
There was a problem hiding this comment.
Pull request overview
This PR refactors the DerefOption<T> type to default to Some(T::default()) instead of None when T implements the Default trait. This change makes it safer to wrap types in DerefOption without changing their default initialization behavior, which helps prevent subtle bugs. The PR also removes an unnecessary T: Debug constraint from the DerefMut implementation.
Key changes:
- Implements a custom
Defaulttrait forDerefOption<T>that createsSome(T::default())whenT: Default - Removes the derived
Defaultimplementation that would have createdNone - Removes unnecessary
T: Debugbound fromDerefMutimplementation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl<T> Default for DerefOption<T> | ||
| where | ||
| T: Default, | ||
| { | ||
| fn default() -> Self { | ||
| Self(Some(T::default())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new Default implementation that returns Some(T::default()) should have test coverage to ensure it behaves as expected. Consider adding a test module similar to other utility files in this directory (e.g., fs_trim.rs, incremental_info.rs). Example test:
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_default_creates_some() {
#[derive(Default, Debug, PartialEq)]
struct TestStruct {
value: i32,
}
let deref_option: DerefOption<TestStruct> = Default::default();
assert_eq!(*deref_option, TestStruct::default());
}
}| impl<T> Default for DerefOption<T> | ||
| where | ||
| T: Default, | ||
| { |
There was a problem hiding this comment.
Consider adding documentation to clarify the behavior of the Default implementation. For example:
/// Creates a `DerefOption<T>` with `Some(T::default())`.
/// This preserves the default behavior when wrapping types that implement `Default`.
impl<T> Default for DerefOption<T>This helps users understand that the default is Some(T::default()) rather than None, which is non-obvious for an Option wrapper.
📦 Binary Size-limit
❌ Size increased by 128bytes from 47.72MB to 47.72MB (⬆️0.00%) |
Rsdoctor Bundle Diff AnalysisFound 5 project(s) in monorepo. 📁 react-10kPath:
📦 Download Diff Report: react-10k Bundle Diff 📁 react-1kPath:
📦 Download Diff Report: react-1k Bundle Diff 📁 react-5kPath:
📦 Download Diff Report: react-5k Bundle Diff 📁 romePath:
📦 Download Diff Report: rome Bundle Diff 📁 ui-componentsPath:
📦 Download Diff Report: ui-components Bundle Diff Generated by Rsdoctor GitHub Action |
CodSpeed Performance ReportMerging #12406 will not alter performanceComparing Summary
|
Why
Change DerefOption to default to Some(T) when T implements the Default trait.
Currently, Artifacts implements Default and is initialized by calling Default::default(). If Artifacts is changed to DerefOption, the existing behavior changes subtly: all fields are initialized to None instead of using Artifacts::default().
This difference in default initialization is easy to miss and can lead to subtle bugs. Defaulting DerefOption to Some(T::default()) when T: Default preserves the original behavior and makes the change safer and more intuitive.
Related links
Checklist