Skip to content
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

Improved trait coherence checking #6844

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Jan 20, 2025

Description

This PR refines trait coherence checking to guarantee that there is only one valid implementation for each trait. It introduces improved orphan rules and more rigorous overlap checks to prevent conflicting trait implementations. With these changes, the system now more reliably enforces coherence, ensuring that traits are implemented uniquely, and
reducing potential ambiguities.

This approach mirrors Rust’s design by enforcing two distinct safety and coherence checks on trait implementations:

1. Orphan Rules Check
The orphan rules require that for any trait implementation, either the trait or the type must be defined within the current package. This restriction prevents external packages from implementing foreign traits for foreign types, which could otherwise lead to conflicting implementations and ambiguities across different parts of a codebase. Essentially, it helps maintain clear ownership and boundaries of trait implementations.

2. Overlap Impl Check
The overlap impl check ensures that no two trait implementations can apply to the same type in an ambiguous manner. If two implementations could potentially match the same type, it would be unclear which one should be used, leading to coherence issues. By enforcing that implementations do not overlap, the system guarantees that trait resolution is unambiguous and predictable.

Together, these checks promote a robust and maintainable system by ensuring that trait implementations are both locally controlled (orphan rules) and non-conflicting (overlap check).

The test suite has been updated to comply with the new rules. However, there is one current limitation regarding arrays. For arrays, the coherence checks have been relaxed to avoid the need for numerous concrete implementations in the standard library for traits like Eq and PartialEq. This decision was made because support for const generics is expected soon, which will allow these traits to be implemented more cleanly.

Closes issue #5892.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@tritao tritao self-assigned this Jan 20, 2025
@tritao tritao added the compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen label Jan 20, 2025
@tritao tritao force-pushed the trait-coherence branch 4 times, most recently from 0bed6b2 to ec4e13e Compare January 27, 2025 10:40
Copy link

codspeed-hq bot commented Feb 3, 2025

CodSpeed Performance Report

Merging #6844 will degrade performances by 50.73%

Comparing tritao:trait-coherence (f770057) with master (fccc8a9)

Summary

❌ 2 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
compile 4.7 s 9.6 s -50.73%
hover 3.4 ms 4 ms -16.54%

This commit refines trait coherence checking to guarantee that there is
only one valid implementation for each trait. It introduces improved
orphan rules and more rigorous overlap checks to prevent conflicting
trait implementations. With these changes, the system now more reliably
enforces coherence, ensuring that traits are implemented uniquely, and
reducing potential ambiguities.

This approach mirrors Rust’s design by enforcing two distinct safety and
coherence checks on trait implementations:

1. Orphan Rules Check
The orphan rules require that for any trait implementation, either the
trait or the type must be defined within the current package. This
restriction prevents external packages from implementing foreign traits
for foreign types, which could otherwise lead to conflicting
implementations and ambiguities across different parts of a codebase.
Essentially, it helps maintain clear ownership and boundaries of trait
implementations.

2. Overlap Impl Check
The overlap impl check ensures that no two trait implementations can
apply to the same type in an ambiguous manner. If two implementations
could potentially match the same type, it would be unclear which one
should be used, leading to coherence issues. By enforcing that
implementations do not overlap, the system guarantees that trait
resolution is unambiguous and predictable.

Together, these checks promote a robust and maintainable system by
ensuring that trait implementations are both locally controlled (orphan
rules) and non-conflicting (overlap check).

The test suite has been updated to comply with the new rules. However,
there is one current limitation regarding arrays. For arrays, the
coherence checks have been relaxed to avoid the need for numerous
concrete implementations in the standard library for traits like `Eq`
and `PartialEq`. This decision was made because support for const
generics is expected soon, which will allow these traits to be
implemented more cleanly.

Closes issue FuelLabs#5892.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant