From fe8534383a6b8906c1ccd22a2a80f324659c8f93 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Wed, 18 Dec 2024 22:55:55 +0200 Subject: [PATCH 1/6] RFC unsized params in traits --- text/3696-unsized-params-in-traits.md | 168 ++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 text/3696-unsized-params-in-traits.md diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md new file mode 100644 index 00000000000..e12e4212056 --- /dev/null +++ b/text/3696-unsized-params-in-traits.md @@ -0,0 +1,168 @@ +- Feature Name: unsized_params_in_traits +- Start Date: 2024-12-18 +- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- Rust Issue: [rust-lang/rust#134475](https://github.com/rust-lang/rust/issues/134475) + +# Summary +[summary]: #summary + +A (temporary) lint which detects unsized parameter in required trait methods, which will become a hard +error in the future + + +# Motivation +[motivation]: #motivation + +This rfc is to prevent the use from making their trait unimplementable (in some cases, for unsized types) + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Think of this lint as the `missing_fragment_specifier` feature (if thats what its called), it is only meant to be temporary and will be a hard error in the future +```rust +#![deny(unsized_params_in_traits)] + +trait Foo { + fn bar(self); +} +``` +the above code fails, because of the lint; this happens because here `Self: ?Sized` + +Also look at this code: +```rust +#![deny(unsized_params_in_traits)] // this is default, but here for clearness + +trait Foo { + fn bar(bytes: [u8]); +} +``` +the above code would work without the lint (how did no one notice this?) + +While both of the above _would_ work without the lint, you cant actually implement it +```rust +impl Foo for i32 { + fn bar(bytes: [u8]) {} +} +``` +Produces: +``` +error: The Size value for `[u8]` is not known at compile time +``` + +So in all: this rfc is to prevent confusion + +Now, if you do notice, in the [summary], i did say `required methods`; provided methods are `Sized`-checked +```rust +trait Foo { + fn bar(self) { + + } +} +``` +Produces: +``` +error: the size value for `Self` is not know at compile time +``` +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +There is one feature this may clash with, and it is the feature for `unsized fn params`, the lint should be disabled when this feature is enabled; if that is possible, however if it may cause confusion to the user. + +Here is the first example, but it clashes with the `unsized fn params` feature +```rust +#![feature(unsized_fn_params)] +// implicit #![deny(unsized_params_in_trait)] +#![allow(internal_features)] + +trait Foo { + fn bar(bytes: [u8]); +} +``` + +The above code fails, while it shouldn't due to the feature + +However, it is internal so it is semi-ok + +this feature will be very simple to implement (i hope), just port the `Sized`-checking logic from provided methods to required methods, if that is possible (also maybe from regular functions) and throw an error/warning/nothing depending on the lint level. + +here is some rust-pseudo code: +```rust +if !req_function. + params + .filter(|param| !param.is_sized()) + .is_empty() { + match lint_level { + Allow => (), + Warn => warn("..."), + Deny | Forbid => error("...") + } + } +``` +replace the `...` with: +``` +The size value for {param_type} is not know at compile time +# ... +This was previously accepted by the compiler, but it will become a hard error in the future! +# if it was the default value of 'deny' the next line would be added +`#[deny(unsized_param_in_trait)]` on by default +``` +Obviously the above code isnt actually correct as it doesnt check _which_ param is unsized, it just checks if there is, (you can probably loop over the 'filter' object, and make an individual error for each one) + +# Drawbacks +[drawbacks]: #drawbacks + +- This could cause breaking changes, however the lint gives time for migration +- This could be intended for `dyn` compatibility (see [rationale-and-alternatives] for a way to fix this) +This drawback is about receivers, take this example +```rust +trait Bar { + fn foo(self); +} +``` +if `self` was sized checked here, and the value _should_ be consumed, then this code would be impossible without a `Self: Sized` bound, but as you know, adding that bound removes `dyn-Compatibility` +```rust +let x: &dyn Bar = &10; +``` +Produces: +``` +Bar is not dyn compatible +# ... +``` + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +This design is best as it helps users to migrate their code before it breaks + +Other Considered Designs: +- as mentioned in [drawbacks], this may be intentional with `receivers` for `dyn` Compatibility +so another design is dividing this lint into two, one for receivers, and one for regular parameters +- Not making it a hard error, which could work but it may cause users to make their traits unimplementable. +- A direct hard error, though not recommended for migration purposes (mentioned above) +- Leaving it be, though again not recommended as mentioned in [motivation] + +The impact of not doing this: +May cause confusion because a parameter is unsized, and the trait cannot be implemented, which is not good. + +This may also cause an `ICE` in some way because the parameters are unsized + +# Prior art +[prior-art]: #prior-art + +This feature is not a problem in other languages, weather a type is `Sized` or not, it is abstracted away and you can never know + +For example: C++ does not really have unsized types (that i know of) +Another: C# abstracts the idea of a `Sized` value + +Higher level languages do not need to run on the machine directly so there is no need to know the size of a value + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +- Should this be a lint, or something else +- Does this need to become a hard error + +# Future possibilities +[future-possibilities]: #future-possibilities + +None, this isnt really something that will stay that long \ No newline at end of file From 9ba1c155c323f6799d9f8cb1a37eed7491871291 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Thu, 19 Dec 2024 09:59:32 +0200 Subject: [PATCH 2/6] Applied changes --- text/3696-unsized-params-in-traits.md | 177 +++++++++++--------------- 1 file changed, 77 insertions(+), 100 deletions(-) diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md index e12e4212056..a0b714a795a 100644 --- a/text/3696-unsized-params-in-traits.md +++ b/text/3696-unsized-params-in-traits.md @@ -1,168 +1,145 @@ -- Feature Name: unsized_params_in_traits -- Start Date: 2024-12-18 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- Feature Name: `unsized_params_in_traits` +- Start Date: 2024-12-19 +- RFC PR: [rust-lang/rfcs#3745](https://github.com/rust-lang/rfcs/pull/3745) - Rust Issue: [rust-lang/rust#134475](https://github.com/rust-lang/rust/issues/134475) # Summary [summary]: #summary -A (temporary) lint which detects unsized parameter in required trait methods, which will become a hard -error in the future - +A future-compatibility lint which will detect unsized parameter types in required trait methods # Motivation [motivation]: #motivation -This rfc is to prevent the use from making their trait unimplementable (in some cases, for unsized types) +To prevent confusion, soft-locking (making a user's trait unimplementable), and to +(possibly) prevent an `ICE` from happening # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -Think of this lint as the `missing_fragment_specifier` feature (if thats what its called), it is only meant to be temporary and will be a hard error in the future +## Without the lint +Rust does not check for `T: Sized` in required trait methods, which makes this code valid: ```rust -#![deny(unsized_params_in_traits)] - trait Foo { - fn bar(self); + fn bar(self, x: str); // compiles } ``` -the above code fails, because of the lint; this happens because here `Self: ?Sized` -Also look at this code: +in the above code, `self` has type `Self`, and `Self: ?Sized`; Function parameters must always be sized + +And x has type `str` which is also not sized + +On top of that, this also prevents a user from implementing this trait for a type ```rust -#![deny(unsized_params_in_traits)] // this is default, but here for clearness +impl Foo for str { + fn bar(self, x: str) {} // err: `str` and `str` are not sized +} +``` +Basically making the trait useless +## With the lint +the lint prevents the user from making this error +```rust +#![deny(unsized_params_in_traits)] trait Foo { - fn bar(bytes: [u8]); + fn bar(self, x: str); // err: `Self` and `str` are not sized } ``` -the above code would work without the lint (how did no one notice this?) - -While both of the above _would_ work without the lint, you cant actually implement it +Now the user is guided to change their types to sized ones ```rust -impl Foo for i32 { - fn bar(bytes: [u8]) {} +trait Foo: Sized // remember this `Self: Sized` bound +{ + fn bar(self, x: String); } ``` -Produces: -``` -error: The Size value for `[u8]` is not known at compile time -``` -So in all: this rfc is to prevent confusion +## Lint +this section contains data about the lint itself -Now, if you do notice, in the [summary], i did say `required methods`; provided methods are `Sized`-checked -```rust -trait Foo { - fn bar(self) { +`Default Level`: `Deny` +`Feature`: `unsized_params_in_traits` (if any, see the unanswered questions) - } -} -``` -Produces: -``` -error: the size value for `Self` is not know at compile time -``` # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -There is one feature this may clash with, and it is the feature for `unsized fn params`, the lint should be disabled when this feature is enabled; if that is possible, however if it may cause confusion to the user. +This is the technical portion of the RFC. Explain the design in sufficient detail that: + +- Its interaction with other features is clear. +- It is reasonably clear how the feature would be implemented. +- Corner cases are dissected by example. -Here is the first example, but it clashes with the `unsized fn params` feature +The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. + +This lint may clash with the `unsized_fn_params` feature (though it is internal), as the entire point of this feature is to allow what +this lint detects. + +look at the first example: ```rust #![feature(unsized_fn_params)] -// implicit #![deny(unsized_params_in_trait)] #![allow(internal_features)] - +// implicit #![deny(unsized_params_in_traits)] trait Foo { - fn bar(bytes: [u8]); + fn bar(self, x: str); } ``` +The code fails even though it shouldn't, which may cause confusion to the user. -The above code fails, while it shouldn't due to the feature +if there is someway to disable this lint by default when `unsized_fn_params` is enabled, it should be implemented. -However, it is internal so it is semi-ok +The above is pretty much only a minor inconvenience, but if the user has many nested crates (like rust itself for example, having std, proc-macro, etc. all as different crates) it may be harder than just one lint. -this feature will be very simple to implement (i hope), just port the `Sized`-checking logic from provided methods to required methods, if that is possible (also maybe from regular functions) and throw an error/warning/nothing depending on the lint level. +## `dyn` compatibility +When dealing with `receiver` the user **may** have meant to have an unsized receiver for `dyn` compatibility[^1] -here is some rust-pseudo code: +While it is confusing, it is still something that is intentional; look at the last example: ```rust -if !req_function. - params - .filter(|param| !param.is_sized()) - .is_empty() { - match lint_level { - Allow => (), - Warn => warn("..."), - Deny | Forbid => error("...") - } - } -``` -replace the `...` with: -``` -The size value for {param_type} is not know at compile time -# ... -This was previously accepted by the compiler, but it will become a hard error in the future! -# if it was the default value of 'deny' the next line would be added -`#[deny(unsized_param_in_trait)]` on by default +trait Foo: Sized { + fn bar(self, x: String); +} ``` -Obviously the above code isnt actually correct as it doesnt check _which_ param is unsized, it just checks if there is, (you can probably loop over the 'filter' object, and make an individual error for each one) + +in the above code, `Foo` has the bound `Self: Sized` which makes it `dyn` incompatible[^1]. So this feature will likely require a different feature to be added along-side it for receivers in specific. + + +[^1]: [...] A trait is object safe if it has the following qualities [...] All associated functions must either be dispatchable from a trait object or be explicitly non-dispatchable: [...] Dispatchable functions must: [...] Not have a where Self: Sized bound (receiver type of Self (i.e. self) implies this). [...] Explicitly non-dispatchable functions require: Have a where Self: Sized bound (receiver type of Self (i.e. self) implies this). # Drawbacks [drawbacks]: #drawbacks -- This could cause breaking changes, however the lint gives time for migration -- This could be intended for `dyn` compatibility (see [rationale-and-alternatives] for a way to fix this) -This drawback is about receivers, take this example -```rust -trait Bar { - fn foo(self); -} -``` -if `self` was sized checked here, and the value _should_ be consumed, then this code would be impossible without a `Self: Sized` bound, but as you know, adding that bound removes `dyn-Compatibility` -```rust -let x: &dyn Bar = &10; -``` -Produces: -``` -Bar is not dyn compatible -# ... -``` +- It may be breaking but it is a future compatibility lint +- `dyn` compatibility of some traits (mentioned above) # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -This design is best as it helps users to migrate their code before it breaks +- Why is this design the best in the space of possible designs? +- What other designs have been considered and what is the rationale for not choosing them? +- What is the impact of not doing this? +- If this is a language proposal, could this be done in a library or macro instead? Does the proposed change make Rust code easier or harder to read, understand, and maintain? + +This design is best as it is a `future-compatibility` lint -Other Considered Designs: -- as mentioned in [drawbacks], this may be intentional with `receivers` for `dyn` Compatibility -so another design is dividing this lint into two, one for receivers, and one for regular parameters -- Not making it a hard error, which could work but it may cause users to make their traits unimplementable. -- A direct hard error, though not recommended for migration purposes (mentioned above) -- Leaving it be, though again not recommended as mentioned in [motivation] +Not doing this would cause confusion, possible soft locking and (possibly) an `ICE` -The impact of not doing this: -May cause confusion because a parameter is unsized, and the trait cannot be implemented, which is not good. +Here are some other considered designs: +- For the dyn compatibility issues, creating a `separate lint` for receivers in specific -This may also cause an `ICE` in some way because the parameters are unsized +There aren't really that much relevant designs as all of them would either not fix the issue or not give enough time for the user to migrate their code # Prior art [prior-art]: #prior-art -This feature is not a problem in other languages, weather a type is `Sized` or not, it is abstracted away and you can never know - -For example: C++ does not really have unsized types (that i know of) -Another: C# abstracts the idea of a `Sized` value - -Higher level languages do not need to run on the machine directly so there is no need to know the size of a value +I haven't really seen anything like this before, rust is pretty-well designed, there are some similar things (e.g. trivial bounds, thanks @oli-obk for that) # Unresolved questions [unresolved-questions]: #unresolved-questions -- Should this be a lint, or something else -- Does this need to become a hard error +- Should this be feature gated? + - I personally answer this with `no` as it is pretty important +- Should this be in clippy instead + - Again, I personally answer this with `no` as it is pretty important # Future possibilities [future-possibilities]: #future-possibilities -None, this isnt really something that will stay that long \ No newline at end of file +None, this will not exist that long anyways, maybe like 3-9 months maximum From 89fe314245690972a4e38ddcecb6abff2aeb1c77 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Thu, 19 Dec 2024 10:06:05 +0200 Subject: [PATCH 3/6] Fixed formatting --- text/3696-unsized-params-in-traits.md | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md index a0b714a795a..04c53f8f8df 100644 --- a/text/3696-unsized-params-in-traits.md +++ b/text/3696-unsized-params-in-traits.md @@ -57,19 +57,12 @@ trait Foo: Sized // remember this `Self: Sized` bound this section contains data about the lint itself `Default Level`: `Deny` + `Feature`: `unsized_params_in_traits` (if any, see the unanswered questions) # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -This is the technical portion of the RFC. Explain the design in sufficient detail that: - -- Its interaction with other features is clear. -- It is reasonably clear how the feature would be implemented. -- Corner cases are dissected by example. - -The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. - This lint may clash with the `unsized_fn_params` feature (though it is internal), as the entire point of this feature is to allow what this lint detects. @@ -112,11 +105,6 @@ in the above code, `Foo` has the bound `Self: Sized` which makes it `dyn` incomp # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -- Why is this design the best in the space of possible designs? -- What other designs have been considered and what is the rationale for not choosing them? -- What is the impact of not doing this? -- If this is a language proposal, could this be done in a library or macro instead? Does the proposed change make Rust code easier or harder to read, understand, and maintain? - This design is best as it is a `future-compatibility` lint Not doing this would cause confusion, possible soft locking and (possibly) an `ICE` From 38427677c11ec17dcc5aadf7d08f56fca007b746 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Thu, 19 Dec 2024 10:10:33 +0200 Subject: [PATCH 4/6] Fixed RFC PR number to new RFC --- text/3696-unsized-params-in-traits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md index 04c53f8f8df..5ff87b115e9 100644 --- a/text/3696-unsized-params-in-traits.md +++ b/text/3696-unsized-params-in-traits.md @@ -1,6 +1,6 @@ - Feature Name: `unsized_params_in_traits` - Start Date: 2024-12-19 -- RFC PR: [rust-lang/rfcs#3745](https://github.com/rust-lang/rfcs/pull/3745) +- RFC PR: [rust-lang/rfcs#3747](https://github.com/rust-lang/rfcs/pull/3747) - Rust Issue: [rust-lang/rust#134475](https://github.com/rust-lang/rust/issues/134475) # Summary From 0b1e4efac3f3c66100e341b569bbcdbfe2e2ca60 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Thu, 19 Dec 2024 10:16:39 +0200 Subject: [PATCH 5/6] Reverted last change --- text/3696-unsized-params-in-traits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md index 5ff87b115e9..04c53f8f8df 100644 --- a/text/3696-unsized-params-in-traits.md +++ b/text/3696-unsized-params-in-traits.md @@ -1,6 +1,6 @@ - Feature Name: `unsized_params_in_traits` - Start Date: 2024-12-19 -- RFC PR: [rust-lang/rfcs#3747](https://github.com/rust-lang/rfcs/pull/3747) +- RFC PR: [rust-lang/rfcs#3745](https://github.com/rust-lang/rfcs/pull/3745) - Rust Issue: [rust-lang/rust#134475](https://github.com/rust-lang/rust/issues/134475) # Summary From 842f59de0d2e37cf0e8bec3540f2e67c5fbbb3b6 Mon Sep 17 00:00:00 2001 From: Mostafa Khaled Date: Thu, 19 Dec 2024 13:41:23 +0200 Subject: [PATCH 6/6] Made feature dependent on `unsized_fn_params` instead --- text/3696-unsized-params-in-traits.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/text/3696-unsized-params-in-traits.md b/text/3696-unsized-params-in-traits.md index 04c53f8f8df..04de2f9e1cc 100644 --- a/text/3696-unsized-params-in-traits.md +++ b/text/3696-unsized-params-in-traits.md @@ -6,7 +6,9 @@ # Summary [summary]: #summary -A future-compatibility lint which will detect unsized parameter types in required trait methods +A lint which will detect unsized parameter types in required trait methods + +it will be removed once feature `unsized_fn_params` is stabilized # Motivation [motivation]: #motivation @@ -63,8 +65,8 @@ this section contains data about the lint itself # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -This lint may clash with the `unsized_fn_params` feature (though it is internal), as the entire point of this feature is to allow what -this lint detects. +This lint may clash with the `unsized_fn_params` feature (though it is internal), as the entire point of +this feature is to allow what this lint detects. look at the first example: ```rust @@ -82,7 +84,7 @@ if there is someway to disable this lint by default when `unsized_fn_params` is The above is pretty much only a minor inconvenience, but if the user has many nested crates (like rust itself for example, having std, proc-macro, etc. all as different crates) it may be harder than just one lint. ## `dyn` compatibility -When dealing with `receiver` the user **may** have meant to have an unsized receiver for `dyn` compatibility[^1] +When dealing with a `receiver` the user **may** have meant to have an unsized receiver for `dyn` compatibility[^1] While it is confusing, it is still something that is intentional; look at the last example: ```rust @@ -99,20 +101,20 @@ in the above code, `Foo` has the bound `Self: Sized` which makes it `dyn` incomp # Drawbacks [drawbacks]: #drawbacks -- It may be breaking but it is a future compatibility lint +- It may be breaking but it is a a lint that will be removed - `dyn` compatibility of some traits (mentioned above) # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -This design is best as it is a `future-compatibility` lint +This design is best as it is a lint, which will be removed when what it detects is stabilized Not doing this would cause confusion, possible soft locking and (possibly) an `ICE` Here are some other considered designs: - For the dyn compatibility issues, creating a `separate lint` for receivers in specific -There aren't really that much relevant designs as all of them would either not fix the issue or not give enough time for the user to migrate their code +There aren't really that much relevant designs as all of them would either not fix the issue or not give enough time for the user to migrate their code # Prior art [prior-art]: #prior-art