From 69a8ed50cc6a87487d9e71e33ff55a1556d521e5 Mon Sep 17 00:00:00 2001 From: Sasha Pourcelot <sasha.pourcelot@iomentum.com> Date: Mon, 11 Oct 2021 12:04:51 +0200 Subject: [PATCH 1/2] Add failing test --- tests/function.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/function.rs b/tests/function.rs index 3facf4e..85c2e01 100644 --- a/tests/function.rs +++ b/tests/function.rs @@ -99,6 +99,22 @@ fn fn_arg_last_character_not_removed() { assert_eq!(diff.to_string(), "≠ a\n"); } +#[test] +fn user_defined_type_change_is_modification() { + let diff = compatibility_diagnosis! { + { + pub struct S; + pub fn f(_: S) {} + }, + { + pub struct S; + pub fn f(_: S) {} + }, + }; + + assert!(diff.is_empty()); +} + #[test] fn empty_struct_kind_change_is_modification() { let files = ["pub struct A;", "pub struct A();", "pub struct A {}"]; From 6a219bfa8e6e94d2e828988b89c9ff1eba41ae8a Mon Sep 17 00:00:00 2001 From: Sasha Pourcelot <sasha.pourcelot@iomentum.com> Date: Mon, 25 Oct 2021 11:43:41 +0200 Subject: [PATCH 2/2] Handle user-defined algebraic data types equality --- src/comparator.rs | 2 +- src/compiler/instrumented_compiler.rs | 14 ++++---- src/public_api.rs | 4 ++- src/public_api/functions.rs | 26 +++++++++++---- src/public_api/utils.rs | 47 +++++++++++++++++++++++++++ tests/function.rs | 2 +- 6 files changed, 80 insertions(+), 15 deletions(-) create mode 100644 src/public_api/utils.rs diff --git a/src/comparator.rs b/src/comparator.rs index cc38512..542809d 100644 --- a/src/comparator.rs +++ b/src/comparator.rs @@ -12,8 +12,8 @@ use crate::{invocation_settings::GlueCompilerInvocationSettings, public_api::Api use semver::{BuildMetadata, Prerelease, Version}; -use rustc_middle::middle::cstore::ExternCrateSource; use rustc_middle::ty::TyCtxt; +use rustc_session::cstore::ExternCrateSource; use rustc_span::def_id::CrateNum; use crate::{diagnosis::DiagnosisItem, public_api::PublicApi}; diff --git a/src/compiler/instrumented_compiler.rs b/src/compiler/instrumented_compiler.rs index 991592d..1dbaeef 100644 --- a/src/compiler/instrumented_compiler.rs +++ b/src/compiler/instrumented_compiler.rs @@ -7,6 +7,7 @@ use crate::{ use anyhow::{anyhow, Context, Result as AnyResult}; use rustc_driver::{Callbacks, Compilation, RunCompiler}; use rustc_interface::Config; +use rustc_middle::ty::TyCtxt; use rustc_session::config::Input; use rustc_span::FileName; use std::{ @@ -208,7 +209,7 @@ impl Callbacks for InstrumentedCompiler { // get prev & next let changeset = queries.global_ctxt().unwrap().take().enter(|tcx| { let comparator = ApiComparator::from_tcx_and_settings(&tcx, settings)?; - get_changeset(&comparator) + get_changeset(&tcx, &comparator) }); *self = InstrumentedCompiler::Finished(changeset); @@ -226,10 +227,10 @@ pub struct ChangeSet { } impl ChangeSet { - pub(crate) fn from_diffs(d: Vec<Diff>) -> Self { + pub(crate) fn from_diffs(tcx: &TyCtxt, d: Vec<Diff>) -> Self { let mut changes = d .into_iter() - .filter_map(Change::from_diff) + .filter_map(|diff| Change::from_diff(tcx, diff)) .map(ExposedChange::from_change) .collect::<Vec<_>>(); @@ -407,7 +408,7 @@ pub(crate) enum Change<'tcx> { } impl<'tcx> Change<'tcx> { - pub(crate) fn from_diff(d: Diff<'tcx>) -> Option<Change<'tcx>> { + pub(crate) fn from_diff(tcx: &TyCtxt, d: Diff<'tcx>) -> Option<Change<'tcx>> { // TODO [sasha]: there's some polymorphism here to perform // to figure out if a change is breaking @@ -421,7 +422,7 @@ impl<'tcx> Change<'tcx> { Some(Change::Breaking(d)) } - Diff::Edition(prev, next) => ApiItem::changes_between(prev, next), + Diff::Edition(prev, next) => ApiItem::changes_between(tcx, prev, next), // Removing anything that is publicly exposed is always breaking. Diff::Deletion(_) => Some(Change::Breaking(d)), @@ -437,10 +438,11 @@ impl<'tcx> Change<'tcx> { } fn get_changeset<'tcx, 'rustc>( + tcx: &TyCtxt, comparator: &'rustc impl Comparator<'tcx, 'rustc>, ) -> AnyResult<ChangeSet> { // get additions / editions / deletions let diffs: Vec<Diff> = comparator.get_diffs(); - Ok(ChangeSet::from_diffs(diffs)) + Ok(ChangeSet::from_diffs(tcx, diffs)) } diff --git a/src/public_api.rs b/src/public_api.rs index 4281bb0..66442c3 100644 --- a/src/public_api.rs +++ b/src/public_api.rs @@ -1,5 +1,6 @@ mod functions; mod modules; +mod utils; use std::{cmp::Ordering, collections::HashMap}; @@ -108,11 +109,12 @@ impl<'tcx> ApiItem<'tcx> { } pub(crate) fn changes_between( + tcx: &TyCtxt, prev: ApiItem<'tcx>, next: ApiItem<'tcx>, ) -> Option<Change<'tcx>> { match (prev, next) { - (ApiItem::Fn(prev), ApiItem::Fn(next)) => FnMetadata::changes_between(prev, next), + (ApiItem::Fn(prev), ApiItem::Fn(next)) => FnMetadata::changes_between(tcx, prev, next), (ApiItem::Mod(prev), ApiItem::Mod(next)) => ModMetadata::changes_between(prev, next), _ => unreachable!("Attempt to generate changes for two different-kinded types"), diff --git a/src/public_api/functions.rs b/src/public_api/functions.rs index 915cea8..18c4b92 100644 --- a/src/public_api/functions.rs +++ b/src/public_api/functions.rs @@ -4,7 +4,7 @@ use rustc_middle::ty::{FnSig, TyCtxt}; use crate::{comparator::Diff, compiler::Change}; -use super::ApiItem; +use super::{utils::Compare, ApiItem}; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct FnMetadata<'tcx> { @@ -30,15 +30,29 @@ impl<'tcx> FnMetadata<'tcx> { } pub(crate) fn changes_between( + tcx: &TyCtxt, prev: FnMetadata<'tcx>, next: FnMetadata<'tcx>, ) -> Option<Change<'tcx>> { if prev.sig != next.sig { - // Adding or removing an argument is *always* a breaking change. - Some(Change::Breaking(Diff::Edition( - prev.to_api_item(), - next.to_api_item(), - ))) + let from_args = + tcx.ty_list_eq(&prev.sig.inputs_and_output, &next.sig.inputs_and_output); + + let is_unchanged = tcx + .ty_list_eq(&prev.sig.inputs_and_output, &next.sig.inputs_and_output) + && prev.sig.c_variadic == next.sig.c_variadic + && prev.sig.unsafety == next.sig.unsafety + && prev.sig.abi == next.sig.abi; + + if !is_unchanged { + // Adding or removing an argument is *always* a breaking change. + Some(Change::Breaking(Diff::Edition( + prev.to_api_item(), + next.to_api_item(), + ))) + } else { + None + } } else { None } diff --git a/src/public_api/utils.rs b/src/public_api/utils.rs new file mode 100644 index 0000000..82a9601 --- /dev/null +++ b/src/public_api/utils.rs @@ -0,0 +1,47 @@ +use std::ptr; + +use rustc_middle::ty::{self, List, TyCtxt, TyKind}; + +pub(crate) trait Compare { + fn ty_eq(&self, left: ty::Ty, right: ty::Ty) -> bool { + ptr::eq(left, right) || self.adt_ty_eq(left, right).unwrap_or_default() + } + + fn ty_list_eq(&self, left: &List<ty::Ty>, right: &List<ty::Ty>) -> bool { + left == right + || (left.len() == right.len() + && left + .iter() + .zip(right.iter()) + .all(|(left, right)| self.ty_eq(left, right))) + } + + /// Returns Some(true) if both left and right are ADTs and refer to the same + /// type. + /// + /// Returns Some(false) if both left and right are ADTs but do not refer to + /// the same type. + /// + /// Returns None if at least one of left or right is not an ADT. + fn adt_ty_eq(&self, left: ty::Ty, right: ty::Ty) -> Option<bool>; +} + +impl<'tcx> Compare for TyCtxt<'tcx> { + fn adt_ty_eq(&self, left: ty::Ty, right: ty::Ty) -> Option<bool> { + let left_path = adt_path_of_ty(self, left)?; + let right_path = adt_path_of_ty(self, right)?; + + Some(left_path == right_path) + } +} + +fn adt_path_of_ty(tcx: &TyCtxt<'_>, ty: ty::Ty) -> Option<String> { + let adt_def_id = match ty.kind() { + TyKind::Adt(adt, _) => adt.did, + _ => return None, + }; + + let path = tcx.def_path(adt_def_id).to_string_no_crate_verbose(); + + Some(path) +} diff --git a/tests/function.rs b/tests/function.rs index 85c2e01..38b557f 100644 --- a/tests/function.rs +++ b/tests/function.rs @@ -100,7 +100,7 @@ fn fn_arg_last_character_not_removed() { } #[test] -fn user_defined_type_change_is_modification() { +fn user_defined_type_change_is_not_reported() { let diff = compatibility_diagnosis! { { pub struct S;