Skip to content

Commit

Permalink
Update error message for improved devX
Browse files Browse the repository at this point in the history
Summary:
update the package error message from
```
 Cannot access an element which belongs to package A from package B [1]
-> This is from a.php, which belongs to package A [2]
-> But b.php is in package B, and package A does not include package B [3]
```
to
```
Cannot access an element which belongs to package A from package B [1]
-> This is from a.php [2]
-> a.php belongs to package package A [3]
-> But b.php is in package B [4]
-> And package A does not include package B [5]
```
where
- [1] tells you the location of the illegal cross-package reference
- [3] points to why the path is in a certain package (either from the __PackageOverride file attribute, or the PACKAGES.toml definition)
- [4] same as [3] but for the file being referenced
- [5] points to the PACKAGES.toml definition (which specifies the package dependencies)

To this end, the diff introduced a `Aast_defs.package_membership` variant that stores whether a symbol's package was derived from __PackageOverride or include_paths globbing, as well as the package name and the position

Reviewed By: francesco-zappa-nardelli

Differential Revision: D66239050

fbshipit-source-id: 933cb0125438acc5ee072a9c70010a65addc0034
  • Loading branch information
Millie Chen authored and facebook-github-bot committed Dec 9, 2024
1 parent bf5b18d commit d5bbd48
Show file tree
Hide file tree
Showing 69 changed files with 956 additions and 484 deletions.
1 change: 1 addition & 0 deletions hphp/hack/src/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions hphp/hack/src/annotated_ast/aast_defs.ml
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,13 @@ and emit_id =
(** Closures are hoisted to classes, but they don't get an entry in .main. *)
[@@transform.opaque]

and package_membership =
| PackageOverride of pos * string
(** Package membership derived from the file attribute __PackageOverride *)
| PackageConfigAssignment of string
(** Package membership derived from the package specification in PACKAGES.toml *)
[@@transform.opaque]

and ('ex, 'en) class_ = {
c_span: pos; [@transform.opaque]
c_annotation: 'en;
Expand Down Expand Up @@ -969,7 +976,7 @@ and ('ex, 'en) class_ = {
c_emit_id: emit_id option;
c_internal: bool;
c_module: sid option;
c_package: string option;
c_package: package_membership option;
}

and class_req = class_hint * require_kind
Expand Down Expand Up @@ -1123,7 +1130,7 @@ and ('ex, 'en) typedef = {
t_module: sid option;
t_docs_url: string option;
t_doc_comment: doc_comment option;
t_package: string option;
t_package: package_membership option;
}

and ('ex, 'en) gconst = {
Expand All @@ -1148,7 +1155,7 @@ and ('ex, 'en) fun_def = {
fd_module: sid option;
fd_tparams: ('ex, 'en) tparam list;
fd_where_constraints: where_constraint_hint list;
fd_package: string option;
fd_package: package_membership option;
}

and ('ex, 'en) module_def = {
Expand Down
5 changes: 5 additions & 0 deletions hphp/hack/src/annotated_ast/aast_utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,8 @@ let expr_to_arg pk e =
match pk with
| Ast_defs.Pnormal -> Anormal e
| Ast_defs.Pinout p -> Ainout (p, e)

let get_package_name = function
| PackageConfigAssignment pkg
| PackageOverride (_, pkg) ->
pkg
2 changes: 2 additions & 0 deletions hphp/hack/src/annotated_ast/aast_utils.mli
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,5 @@ val arg_to_expr : ('a, 'b) Aast_defs.argument -> ('a, 'b) Aast_defs.expr
(** Convert an an expression to an argument, using the supplied inout *)
val expr_to_arg :
Ast_defs.param_kind -> ('a, 'b) Aast_defs.expr -> ('a, 'b) Aast_defs.argument

val get_package_name : Aast_defs.package_membership -> string
2 changes: 1 addition & 1 deletion hphp/hack/src/decl/decl_defs.ml
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ type decl_class_type = {
dc_sort_text: string option;
(** The string provided by the __AutocompleteSortText attribute used for sorting
autocomplete results. *)
dc_package: string option;
dc_package: Aast_defs.package_membership option;
}
[@@deriving show]

Expand Down
36 changes: 23 additions & 13 deletions hphp/hack/src/decl/direct_decl_smart_constructors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use namespaces_rust as namespaces;
use naming_special_names_rust as naming_special_names;
use oxidized::decl_parser_options::DeclParserOptions;
use oxidized_by_ref::aast;
use oxidized_by_ref::aast_defs::PackageMembership;
use oxidized_by_ref::ast_defs::Abstraction;
use oxidized_by_ref::ast_defs::Bop;
use oxidized_by_ref::ast_defs::ClassishKind;
Expand Down Expand Up @@ -139,7 +140,7 @@ pub struct Impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> {
inside_no_auto_dynamic_class: bool,
source_text_allocator: S,
pub module: Option<Id<'a>>,
package: Option<&'a str>,
package: Option<&'a PackageMembership>,
}

impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> DirectDeclSmartConstructors<'a, 'o, 't, S> {
Expand All @@ -161,7 +162,10 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> DirectDeclSmartConstructors<'a,
.get_package_for_file(opts.package_v2_support_multifile_tests, path)
{
Some(s) => {
let package = bump::String::from_str_in(s, arena).into_bump_str();
let package_name = bump::String::from_str_in(s, arena).into_bump_str();
let package: &PackageMembership = arena.alloc(
PackageMembership::PackageConfigAssignment(String::from(package_name)),
);
Some(package)
}
None => None,
Expand Down Expand Up @@ -3696,7 +3700,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
attributes: user_attributes,
internal,
docs_url,
package: self.package,
package: self.package.clone(),
});

let this = Rc::make_mut(&mut self.state);
Expand Down Expand Up @@ -3770,7 +3774,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
attributes: user_attributes,
internal: false,
docs_url: None,
package: self.package,
package: self.package.clone(),
});

let this = Rc::make_mut(&mut self.state);
Expand Down Expand Up @@ -3878,7 +3882,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
attributes: user_attributes,
internal,
docs_url,
package: self.package,
package: self.package.clone(),
});

let this = Rc::make_mut(&mut self.state);
Expand Down Expand Up @@ -4146,7 +4150,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
|| parsed_attributes.dynamically_callable,
no_auto_dynamic: self.under_no_auto_dynamic,
no_auto_likes: parsed_attributes.no_auto_likes,
package: self.package,
package: self.package.clone(),
});
let this = Rc::make_mut(&mut self.state);
this.add_fun(name, fun_elt);
Expand Down Expand Up @@ -4839,7 +4843,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
user_attributes,
enum_type: None,
docs_url,
package: self.package,
package: self.package.clone(),
});
let this = Rc::make_mut(&mut self.state);
this.add_class(name, cls);
Expand Down Expand Up @@ -5319,7 +5323,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
includes,
})),
docs_url,
package: self.package,
package: self.package.clone(),
});
let this = Rc::make_mut(&mut self.state);
this.add_class(key, cls);
Expand Down Expand Up @@ -5538,7 +5542,7 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
includes,
})),
docs_url,
package: self.package,
package: self.package.clone(),
});
let this = Rc::make_mut(&mut self.state);
this.add_class(name.1, cls);
Expand Down Expand Up @@ -6641,16 +6645,22 @@ impl<'a, 'o, 't, S: SourceTextAllocator<'t, 'a>> FlattenSmartConstructors
_right_double_angle: Self::Output,
) -> Self::Output {
let keep_user_attributes = self.opts.keep_user_attributes;
let self_cloned = self.clone();
let this = Rc::make_mut(&mut self.state);
this.file_attributes = List::empty();
for attr in attributes.iter() {
match attr {
Node::Attribute(attr) => {
if attr.name.1 == naming_special_names::user_attributes::PACKAGE_OVERRIDE {
if let [AttributeParam::String(_, s)] = &attr.params {
this.package = Some(
std::str::from_utf8(s).expect("Unable to parse package override"),
);
if let &[AttributeParam::String(pos, s)] = attr.params {
let package_name =
std::str::from_utf8(s).expect("Unable to parse package override");
let package_override =
self_cloned.alloc(PackageMembership::PackageOverride(
pos.clone().into(),
package_name.into(),
));
this.package = Some(package_override);
}
}
if keep_user_attributes {
Expand Down
2 changes: 1 addition & 1 deletion hphp/hack/src/decl/shallow_decl_defs.ml
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ type shallow_class = {
sc_user_attributes: user_attribute list;
sc_enum_type: enum_type option;
sc_docs_url: string option;
sc_package: string option;
sc_package: Aast_defs.package_membership option;
}
[@@deriving eq, show]

Expand Down
2 changes: 1 addition & 1 deletion hphp/hack/src/decl/shallow_decl_defs.mli
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ type shallow_class = {
sc_user_attributes: user_attribute list;
sc_enum_type: enum_type option;
sc_docs_url: string option;
sc_package: string option;
sc_package: Aast_defs.package_membership option;
}
[@@deriving eq, show]

Expand Down
5 changes: 4 additions & 1 deletion hphp/hack/src/elab/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// This source code is licensed under the MIT license found in the
// LICENSE file in the "hack" directory of this source tree.
//
// @generated SignedSource<<c316e9b317a31f816b424e5a080f1e99>>
// @generated SignedSource<<37ff048d2395050fe48baee6b338d2f2>>
//
// To regenerate this file, run:
// hphp/hack/src/oxidized_regen.sh
Expand Down Expand Up @@ -1315,6 +1315,9 @@ const _: () = {
const _: () = {
impl Transform for EmitId {}
};
const _: () = {
impl Transform for PackageMembership {}
};
impl Transform for Class_ {
fn transform(&mut self, env: &Env, pass: &mut (impl Pass + Clone)) {
let mut in_pass = pass.clone();
Expand Down
3 changes: 2 additions & 1 deletion hphp/hack/src/hackrs/ty/decl/folded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use hash::IndexMap;
use hash::IndexSet;
use ocamlrep::FromOcamlRep;
use ocamlrep::ToOcamlRep;
use oxidized::ast::PackageMembership;
pub use oxidized::ast_defs::Abstraction;
pub use oxidized::ast_defs::ClassishKind;
use pos::Bytes;
Expand Down Expand Up @@ -223,7 +224,7 @@ pub struct FoldedClass<R: Reason> {
pub allow_multiple_instantiations: bool,
/// The string provided by the <<__AutocompleteSortText>> attribute.
pub sort_text: Option<String>,
pub package: Option<String>,
pub package: Option<PackageMembership>,
}

impl<R: Reason> FoldedClass<R> {
Expand Down
8 changes: 4 additions & 4 deletions hphp/hack/src/hackrs/ty/decl/from_oxidized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ impl<R: Reason> From<&obr::shallow_decl_defs::ClassDecl<'_>> for shallow::Shallo
user_attributes: slice(user_attributes),
enum_type: enum_type.map(Into::into),
docs_url: docs_url.map(Into::into),
package: package.map(Into::into),
package: package.cloned(),
}
}
}
Expand All @@ -497,7 +497,7 @@ impl<R: Reason> From<&obr::shallow_decl_defs::FunDecl<'_>> for shallow::FunDecl<
support_dynamic_type: sf.support_dynamic_type,
no_auto_dynamic: sf.no_auto_dynamic,
no_auto_likes: sf.no_auto_likes,
package: sf.package.map(Into::into),
package: sf.package.cloned(),
}
}
}
Expand All @@ -515,7 +515,7 @@ impl<R: Reason> From<&obr::shallow_decl_defs::TypedefDecl<'_>> for shallow::Type
attributes: slice(x.attributes),
internal: x.internal,
docs_url: x.docs_url.map(Into::into),
package: x.package.map(Into::into),
package: x.package.cloned(),
}
}
}
Expand Down Expand Up @@ -792,7 +792,7 @@ impl<R: Reason> From<&obr::decl_defs::DeclClassType<'_>> for folded::FoldedClass
docs_url: docs_url.map(Into::into),
allow_multiple_instantiations: *allow_multiple_instantiations,
sort_text: sort_text.map(Into::into),
package: package.map(Into::into),
package: package.cloned(),
}
}
}
2 changes: 1 addition & 1 deletion hphp/hack/src/hackrs/ty/decl/shallow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ pub struct ShallowClass<R: Reason> {
pub user_attributes: Box<[UserAttribute<R::Pos>]>,
pub enum_type: Option<EnumType<R>>,
pub docs_url: Option<String>,
pub package: Option<String>,
pub package: Option<oxidized::aast_defs::PackageMembership>,
}

walkable!(ShallowClass<R> as visit_shallow_class => [
Expand Down
10 changes: 4 additions & 6 deletions hphp/hack/src/hackrs/ty/decl/to_oxidized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ impl<'a, R: Reason> ToOxidized<'a> for folded::FoldedClass<R> {
docs_url: docs_url.as_deref().to_oxidized(arena),
allow_multiple_instantiations: *allow_multiple_instantiations,
sort_text: sort_text.as_deref().to_oxidized(arena),
package: package.as_deref().to_oxidized(arena),
package: package.as_ref().map(|p| p.to_arena_allocated(arena)),
})
}
}
Expand Down Expand Up @@ -780,9 +780,7 @@ impl<'a, R: Reason> ToOxidized<'a> for shallow::ClassDecl<R> {
docs_url: docs_url
.as_ref()
.map(|s| bumpalo::collections::String::from_str_in(s, arena).into_bump_str()),
package: package
.as_ref()
.map(|s| bumpalo::collections::String::from_str_in(s, arena).into_bump_str()),
package: package.as_ref().map(|p| p.to_arena_allocated(arena)),
})
}
}
Expand Down Expand Up @@ -819,7 +817,7 @@ impl<'a, R: Reason> ToOxidized<'a> for shallow::FunDecl<R> {
let (pos, id) = m.to_oxidized(arena);
obr::ast_defs::Id(pos, id)
}),
package: package.as_deref().to_oxidized(arena),
package: package.as_ref().map(|p| p.to_arena_allocated(arena)),
})
}
}
Expand Down Expand Up @@ -855,7 +853,7 @@ impl<'a, R: Reason> ToOxidized<'a> for shallow::TypedefDecl<R> {
attributes: attributes.to_oxidized(arena),
internal: *internal,
docs_url: docs_url.as_deref().to_oxidized(arena),
package: package.as_deref().to_oxidized(arena),
package: package.as_ref().map(|p| p.to_arena_allocated(arena)),
})
}
}
Expand Down
7 changes: 5 additions & 2 deletions hphp/hack/src/hackrs/ty/decl/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use hcons::Hc;
use ocamlrep::FromOcamlRep;
use ocamlrep::ToOcamlRep;
use oxidized::aast;
pub use oxidized::aast_defs::PackageMembership;
pub use oxidized::aast_defs::ReifyKind;
pub use oxidized::aast_defs::Tprim as Prim;
use oxidized::ast_defs;
Expand Down Expand Up @@ -659,13 +660,15 @@ pub struct ConstDecl<R: Reason> {

walkable!(ConstDecl<R> => [ty]);

walkable!(PackageMembership);

#[derive(Clone, Debug, Eq, EqModuloPos, Hash, PartialEq, Serialize, Deserialize)]
#[derive(ToOcamlRep, FromOcamlRep)]
#[serde(bound = "R: Reason")]
pub struct FunElt<R: Reason> {
pub deprecated: Option<Bytes>,
pub module: Option<Positioned<ModuleName, R::Pos>>,
pub package: Option<String>,
pub package: Option<PackageMembership>,
/// Top-level functions have limited visibilities
pub internal: bool,
pub ty: Ty<R>,
Expand Down Expand Up @@ -745,7 +748,7 @@ pub struct TypedefType<R: Reason> {
pub attributes: Box<[UserAttribute<R::Pos>]>,
pub internal: bool,
pub docs_url: Option<String>,
pub package: Option<String>,
pub package: Option<PackageMembership>,
}

walkable!(TypedefType<R> => [tparams, as_constraint, super_constraint, type_assignment]);
Expand Down
2 changes: 2 additions & 0 deletions hphp/hack/src/hh_codegen/common/by_ref_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub fn node_impl() -> TokenStream {
impl<'a> Node<'a> for isize {}
impl<'a> Node<'a> for str {}
impl<'a> Node<'a> for bstr::BStr {}
impl<'a> Node<'a> for crate::aast_defs::PackageMembership {}
impl<'a> Node<'a> for crate::file_info::Mode {}
impl<'a> Node<'a> for crate::local_id::LocalId<'a> {}
impl<'a> Node<'a> for crate::method_flags::MethodFlags {}
Expand All @@ -36,6 +37,7 @@ pub fn node_impl() -> TokenStream {
impl<'a> Node<'a> for crate::tany_sentinel::TanySentinel {}
impl<'a> Node<'a> for crate::typing_defs_flags::FunParamFlags {}
impl<'a> Node<'a> for crate::typing_defs_flags::FunTypeFlags {}
impl<'a> Node<'a> for rc_pos::Pos {}

impl<'a, T: Node<'a> + ?Sized> Node<'a> for &'a T {
fn recurse(&'a self, v: &mut dyn Visitor<'a>) {
Expand Down
Loading

0 comments on commit d5bbd48

Please sign in to comment.