Skip to content

Commit

Permalink
chore: check for references to private functions during path resoluti…
Browse files Browse the repository at this point in the history
…on (#4622)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR pulls out the actual changes to path resolution from #4491,
omitting the changes which add visibility modifiers to modules.

We now determine whether a function is private (and so a warning or
error should be emitted) during def collection. This is necessary as to
properly determine whether a function is public or private we need
information on whether all the modules in the function's path relative
to where it's being used allow its contents to be used from this module
- information which doesn't exist in the typechecking pass.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Apr 2, 2024
1 parent 6ee645a commit 420e56d
Show file tree
Hide file tree
Showing 7 changed files with 243 additions and 153 deletions.
23 changes: 17 additions & 6 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::graph::CrateId;
use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId};
use crate::hir::resolution::errors::ResolverError;

use crate::hir::resolution::import::{resolve_import, ImportDirective};
use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution};
use crate::hir::resolution::{
collect_impls, collect_trait_impls, path_resolver, resolve_free_functions, resolve_globals,
resolve_impls, resolve_structs, resolve_trait_by_path, resolve_trait_impls, resolve_traits,
Expand Down Expand Up @@ -56,8 +56,11 @@ impl UnresolvedFunctions {
for bound in &mut func.def.where_clause {
match resolve_trait_by_path(def_maps, module, bound.trait_bound.trait_path.clone())
{
Ok(trait_id) => {
Ok((trait_id, warning)) => {
bound.trait_bound.trait_id = Some(trait_id);
if let Some(warning) = warning {
errors.push(DefCollectorErrorKind::PathResolutionError(warning));
}
}
Err(err) => {
errors.push(err);
Expand Down Expand Up @@ -281,6 +284,13 @@ impl DefCollector {
for collected_import in def_collector.collected_imports {
match resolve_import(crate_id, &collected_import, &context.def_maps) {
Ok(resolved_import) => {
if let Some(error) = resolved_import.error {
errors.push((
DefCollectorErrorKind::PathResolutionError(error).into(),
root_file_id,
));
}

// Populate module namespaces according to the imports used
let current_def_map = context.def_maps.get_mut(&crate_id).unwrap();

Expand All @@ -299,9 +309,9 @@ impl DefCollector {
}
}
}
Err((error, module_id)) => {
Err(error) => {
let current_def_map = context.def_maps.get(&crate_id).unwrap();
let file_id = current_def_map.file_id(module_id);
let file_id = current_def_map.file_id(collected_import.module_id);
let error = DefCollectorErrorKind::PathResolutionError(error);
errors.push((error.into(), file_id));
}
Expand Down Expand Up @@ -409,12 +419,13 @@ fn inject_prelude(
Path { segments: segments.clone(), kind: crate::PathKind::Dep, span: Span::default() };

if !crate_id.is_stdlib() {
if let Ok(module_def) = path_resolver::resolve_path(
if let Ok(PathResolution { module_def_id, error }) = path_resolver::resolve_path(
&context.def_maps,
ModuleId { krate: crate_id, local_id: crate_root },
path,
) {
let module_id = module_def.as_module().expect("std::prelude should be a module");
assert!(error.is_none(), "Tried to add private item to prelude");
let module_id = module_def_id.as_module().expect("std::prelude should be a module");
let prelude = context.module(module_id).scope().names();

for path in prelude {
Expand Down
13 changes: 1 addition & 12 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub enum ResolverError {
#[error("path is not an identifier")]
PathIsNotIdent { span: Span },
#[error("could not resolve path")]
PathResolutionError(PathResolutionError),
PathResolutionError(#[from] PathResolutionError),
#[error("Expected")]
Expected { span: Span, expected: String, got: String },
#[error("Duplicate field in constructor")]
Expand Down Expand Up @@ -72,10 +72,6 @@ pub enum ResolverError {
NumericConstantInFormatString { name: String, span: Span },
#[error("Closure environment must be a tuple or unit type")]
InvalidClosureEnvironment { typ: Type, span: Span },
#[error("{name} is private and not visible from the current module")]
PrivateFunctionCalled { name: String, span: Span },
#[error("{name} is not visible from the current crate")]
NonCrateFunctionCalled { name: String, span: Span },
#[error("Nested slices are not supported")]
NestedSlices { span: Span },
#[error("#[recursive] attribute is only allowed on entry points to a program")]
Expand Down Expand Up @@ -290,13 +286,6 @@ impl From<ResolverError> for Diagnostic {
ResolverError::InvalidClosureEnvironment { span, typ } => Diagnostic::simple_error(
format!("{typ} is not a valid closure environment type"),
"Closure environment must be a tuple or unit type".to_string(), span),
// This will be upgraded to an error in future versions
ResolverError::PrivateFunctionCalled { span, name } => Diagnostic::simple_warning(
format!("{name} is private and not visible from the current module"),
format!("{name} is private"), span),
ResolverError::NonCrateFunctionCalled { span, name } => Diagnostic::simple_warning(
format!("{name} is not visible from the current crate"),
format!("{name} is only visible within its crate"), span),
ResolverError::NestedSlices { span } => Diagnostic::simple_error(
"Nested slices are not supported".into(),
"Try to use a constant sized array instead".into(),
Expand Down
Loading

0 comments on commit 420e56d

Please sign in to comment.