Skip to content

feat: Implement object safety and its hovering hint #17814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

ShoyuVanilla
Copy link
Member

@ShoyuVanilla ShoyuVanilla commented Aug 6, 2024

Resolves #17779

  • Fill missing implementations
  • Hover rendering
  • Implement object safety's own test suite, like layout
  • Add test cases (from rustc maybe)
  • Clean up ugly codes
  • Add doc string

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2024
@bors
Copy link
Contributor

bors commented Aug 8, 2024

☔ The latest upstream changes (presumably #17832) made this pull request unmergeable. Please resolve the merge conflicts.

@ShoyuVanilla
Copy link
Member Author

It might take some time because I'm having difficulties re-implementing some functionality in rustc into r-a 😅 (I think I would do it eventually)

@bors
Copy link
Contributor

bors commented Aug 15, 2024

☔ The latest upstream changes (presumably #17893) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Aug 23, 2024

☔ The latest upstream changes (presumably #17936) made this pull request unmergeable. Please resolve the merge conflicts.

@ShoyuVanilla ShoyuVanilla force-pushed the object-safety branch 3 times, most recently from 1926ed3 to 9fe56f7 Compare August 25, 2024 15:47
@ShoyuVanilla
Copy link
Member Author

I think that basic implementation is complete. Now it's test time

@ShoyuVanilla ShoyuVanilla force-pushed the object-safety branch 2 times, most recently from 461e21a to fd8afe2 Compare August 25, 2024 16:43
@ShoyuVanilla
Copy link
Member Author

While testing this plugged into here;

fn is_object_safe(&self, _trait_id: chalk_ir::TraitId<Interner>) -> bool {
// FIXME: implement actual object safety
true
}

I've found some minor mistakes in my implementation and fixes for them 😅 (All the test are green in my local revision with the above things applied)
I'll push them some hours later

@bors
Copy link
Contributor

bors commented Aug 26, 2024

☔ The latest upstream changes (presumably #17941) made this pull request unmergeable. Please resolve the merge conflicts.

@ShoyuVanilla ShoyuVanilla force-pushed the object-safety branch 3 times, most recently from dc50b0f to 33ebf59 Compare August 26, 2024 17:12
@bors
Copy link
Contributor

bors commented Aug 27, 2024

☔ The latest upstream changes (presumably #17972) made this pull request unmergeable. Please resolve the merge conflicts.

fn take(self, n: usize) -> crate::iter::Take<Self>
where
Self: Sized,
{
Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std has this bound, too (and without this, Iterator is not object safe)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought every relevant test from https://github.com/rust-lang/rust/tree/ae9f5019f9ce6eb3ecd96206ade4a612efe20fd5/tests/ui/object-safety except one with HRTB - it's still checked as object unsafe as it is in rustc but reason is different because we don't support HRBT yet -

@ShoyuVanilla ShoyuVanilla marked this pull request as ready for review August 27, 2024 12:28
@ShoyuVanilla
Copy link
Member Author

Things to do after this(not right after 😅 except the first one);

  • Implement diagnostics for E0038
  • Implement diagnostics for non-dispatchable item usage in the trait object like
trait Foo {
    fn foo(&self)
    where
        Self: Sized;
}

fn test(x: &dyn Foo) {
    x.foo();
      ^^^
}
  • (Maybe) Implement RTPITIT in similar way as in rustc and check for object safety in that way
  • Implement HRBT and check object safety for it

@ShoyuVanilla ShoyuVanilla force-pushed the object-safety branch 2 times, most recently from 19b38de to 19f6a3f Compare August 28, 2024 14:16
name.as_str()
);
let desc = match mvc {
MethodViolationCode::StaticMethod => "static (no receiver parameter)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MethodViolationCode::StaticMethod => "static (no receiver parameter)",
MethodViolationCode::StaticMethod => "missing a receiver",

static isn't too descriptive I think (and might confuse people with a static item)

let name = hir::Function::from(func).name(db);
format_to!(
buf,
"has a method `{}` that is non dispatchable because of;\n// - ",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"has a method `{}` that is non dispatchable because of;\n// - ",
"has a method `{}` that is non dispatchable because of:\n// - ",

MethodViolationCode::ReferencesImplTraitInTrait => {
"the return type contains `impl Trait`"
}
MethodViolationCode::AsyncFn => "async",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MethodViolationCode::AsyncFn => "async",
MethodViolationCode::AsyncFn => "being async",

MethodViolationCode::WhereClauseReferencesSelf => {
"a where clause references `Self`"
}
MethodViolationCode::Generic => "a Generic parameter other than lifetime",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MethodViolationCode::Generic => "a Generic parameter other than lifetime",
MethodViolationCode::Generic => "a non-lifetime generic parameter",

"a where clause references `Self`"
}
MethodViolationCode::Generic => "a Generic parameter other than lifetime",
MethodViolationCode::UndispatchableReceiver => "the non dispatchable receiver type",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MethodViolationCode::UndispatchableReceiver => "the non dispatchable receiver type",
MethodViolationCode::UndispatchableReceiver => "a non-dispatchable receiver type",

@Veykril
Copy link
Member

Veykril commented Aug 29, 2024

Couple of wording nits, otherwise lgtm!
@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 29, 2024

✌️ @ShoyuVanilla, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@ShoyuVanilla
Copy link
Member Author

@bors r=@Veykril

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 6520a43 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 29, 2024

⌛ Testing commit 6520a43 with merge 0ae42bd...

@bors
Copy link
Contributor

bors commented Aug 29, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 0ae42bd to master...

@bors bors merged commit 0ae42bd into rust-lang:master Aug 29, 2024
11 checks passed
@ShoyuVanilla ShoyuVanilla deleted the object-safety branch August 29, 2024 13:39
@ShoyuVanilla
Copy link
Member Author

This caused some regressions only in diesel. I'll find out why 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hovering a trait should tell whether it is object safe or not
4 participants