Skip to content

Commit

Permalink
Add ops::* to the prelude in core (FuelLabs#3892)
Browse files Browse the repository at this point in the history
This PR fixes the bug described in FuelLabs#3891 by adding `ops` to the prelude
in `core`. This change is congruent with Rust's approach as well.

This is a _breaking change_---if any users have any traits manually
implemented for which core already has an existing implementation, this
will now create an error. A rightful one, IMO, but a breaking change
nonetheless.

A few of the tests were written in a way that is not compatible with
this change---so this PR rewrites these. The only one of note is that a
old `should_pass/language/trait_override_bug` test was refactored to a
new `should_fail/redefine_method_from_core` test.

This PR also refactors the trait finding algorithm in
`check_if_trait_constraints_are_satisfied_for_type` to reduce it from
O(n^2) to O(n).

Closes FuelLabs#3891

Co-authored-by: emilyaherbert <[email protected]>
  • Loading branch information
emilyaherbert and emilyaherbert authored Jan 26, 2023
1 parent ed1bc39 commit 0ab1d22
Show file tree
Hide file tree
Showing 13 changed files with 86 additions and 93 deletions.
83 changes: 52 additions & 31 deletions sway-core/src/semantic_analysis/namespace/trait_map.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::BTreeSet;
use std::collections::{BTreeMap, BTreeSet};

use sway_error::error::CompileError;
use sway_types::{Ident, Span, Spanned};
Expand Down Expand Up @@ -711,30 +711,11 @@ impl TraitMap {
let type_engine = engines.te();
let decl_engine = engines.de();

let required_traits: BTreeSet<Ident> = constraints
let all_impld_traits: BTreeMap<Ident, TypeId> = self
.trait_impls
.iter()
.cloned()
.map(|constraint| constraint.trait_name.suffix)
.collect();
let mut found_traits: BTreeSet<Ident> = BTreeSet::new();

for constraint in constraints.iter() {
let TraitConstraint {
trait_name: constraint_trait_name,
type_arguments: constraint_type_arguments,
} = constraint;
let constraint_type_id = type_engine.insert(
decl_engine,
TypeInfo::Custom {
name: constraint_trait_name.suffix.clone(),
type_arguments: if constraint_type_arguments.is_empty() {
None
} else {
Some(constraint_type_arguments.clone())
},
},
);
for key in self.trait_impls.iter().map(|e| &e.key) {
.filter_map(|e| {
let key = &e.key;
let suffix = &key.name.suffix;
let map_trait_type_id = type_engine.insert(
decl_engine,
Expand All @@ -747,15 +728,55 @@ impl TraitMap {
},
},
);
if are_equal_minus_dynamic_types(engines, type_id, key.type_id)
&& are_equal_minus_dynamic_types(engines, constraint_type_id, map_trait_type_id)
{
found_traits.insert(constraint_trait_name.suffix.clone());
if are_equal_minus_dynamic_types(engines, type_id, key.type_id) {
Some((suffix.name.clone(), map_trait_type_id))
} else {
None
}
}
}
})
.collect();

let required_traits: BTreeMap<Ident, TypeId> = constraints
.iter()
.map(|c| {
let TraitConstraint {
trait_name: constraint_trait_name,
type_arguments: constraint_type_arguments,
} = c;
let constraint_type_id = type_engine.insert(
decl_engine,
TypeInfo::Custom {
name: constraint_trait_name.suffix.clone(),
type_arguments: if constraint_type_arguments.is_empty() {
None
} else {
Some(constraint_type_arguments.clone())
},
},
);
(c.trait_name.suffix.clone(), constraint_type_id)
})
.collect();

let relevant_impld_traits: BTreeMap<Ident, TypeId> = all_impld_traits
.into_iter()
.filter(|(impld_trait_name, impld_trait_type_id)| {
match required_traits.get(impld_trait_name) {
Some(constraint_type_id) => are_equal_minus_dynamic_types(
engines,
*constraint_type_id,
*impld_trait_type_id,
),
_ => false,
}
})
.collect();

let required_traits_names: BTreeSet<Ident> = required_traits.keys().cloned().collect();
let relevant_impld_traits_names: BTreeSet<Ident> =
relevant_impld_traits.keys().cloned().collect();

for trait_name in required_traits.difference(&found_traits) {
for trait_name in required_traits_names.difference(&relevant_impld_traits_names) {
// TODO: use a better span
errors.push(CompileError::TraitConstraintNotSatisfied {
ty: engines.help_out(type_id).to_string(),
Expand Down
1 change: 1 addition & 0 deletions sway-lib-core/src/prelude.sw
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ use ::primitives::*;
use ::raw_ptr::*;
use ::raw_slice::*;
use ::never::*;
use ::ops::*;
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
[[package]]
name = 'core'
source = 'path+from-root-19202DB015C3C015'
source = 'path+from-root-3CA8975A62B42346'

[[package]]
name = 'trait_override_bug'
name = 'redefine_method_from_core'
source = 'member'
dependencies = ['core']
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
[project]
name = "trait_override_bug"
name = "redefine_method_from_core"
authors = ["Fuel Labs <[email protected]>"]
entry = "main.sw"
license = "Apache-2.0"

[dependencies]
core = { path = "../../../../../../../sway-lib-core" }
core = { path = "../../../../../../sway-lib-core" }
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
script;

// This bug was found by Nebula in discord. Adding this trait was causing other implementations on u64s to be overridden.

pub trait Shiftable {
fn lsh(self, other: u64) -> Self;
fn rsh(self, other: u64) -> Self;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
category = "fail"

# check: $()error
# nextln: main.sw:9:1
# check: $()impl Shiftable for u64 {
# check: $()Conflicting implementations of trait "Shiftable" for type "u64".
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ fn simple_option_generics_test() {
assert(get_an_option::<u64>().is_none());
}

fn test_assert_eq_u64() {
let a = 42;
let b = 40 + 2;
assert_eq(a, b);
}

fn test_try_from() {
let x = u64::try_from(7);
assert(x.unwrap() == 42);
Expand All @@ -56,6 +62,8 @@ fn main() {
simple_vec_test();
complex_vec_test();
simple_option_generics_test();
test_assert_eq_u64();
test_try_from();
}

fn sell_product() -> MyResult<bool, CustomType> {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
library utils;

use std::revert::revert;

pub fn vec_from(vals: [u32; 3]) -> Vec<u32> {
let mut vec = Vec::new();
vec.push(vals[0]);
Expand All @@ -12,6 +14,12 @@ pub fn get_an_option<T>() -> Option<T> {
Option::None
}

pub fn assert_eq<T>(v1: T, v2: T) where T: Eq {
if (v1 != v2) {
revert(0xffff_ffff_ffff_0004);
}
}

pub trait TryFrom<T> {
fn try_from(b: T) -> Option<Self>;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,6 @@ script;

fn main() {}

pub trait Shiftable {
fn lsh(self, other: Self) -> Self;
fn rsh(self, other: Self) -> Self;
}

impl Shiftable for u64 {
fn lsh(self, other: u64) -> Self {
asm(r1 : self, r2: other, r3) {
sll r3 r1 r2;
r3: u64
}
}

fn rsh(self, other: u64) -> Self {
asm(r1 : self, r2: other, r3) {
srl r3 r1 r2;
r3: u64
}
}
}

fn sqrt(gas_: u64, amount_: u64, coin_: b256, value: u64)-> u64 {
let mut z:u64 = 1;
let mut y:u64 = value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ use shiftable::*;
fn main() {
let mut shiftAnswer: u64 = 0;

shiftAnswer.rsh(5);
shiftAnswer.my_rsh(5);
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
library shiftable;
pub trait Shiftable {
fn lsh(self, other: Self) -> Self;
fn rsh(self, other: Self) -> Self;
pub trait MyShiftable {
fn my_lsh(self, other: Self) -> Self;
fn my_rsh(self, other: Self) -> Self;
}

impl Shiftable for u64 {
fn lsh(self, other: u64) -> Self {
impl MyShiftable for u64 {
fn my_lsh(self, other: u64) -> Self {
asm(r1: self, r2: other, r3) {
sll r3 r1 r2;
r3: u64
}
}
fn rsh(self, other: u64) -> Self {
fn my_rsh(self, other: u64) -> Self {
asm(r1: self, r2: other, r3) {
srl r3 r1 r2;
r3: u64
Expand Down

This file was deleted.

This file was deleted.

0 comments on commit 0ab1d22

Please sign in to comment.