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

Add late name resolution pass #2620

Closed
wants to merge 26 commits into from

Conversation

CohenArthur
Copy link
Member

This is a massive draft PR which I'll split in multiple tinier ones

  • expected: nodiscard
  • optional: Add [[nodiscard]]
  • late
  • sesh: Add late name resolution 2.0
  • session manager
  • ast: change SimplePath, PathInExpression etc's API
  • default: Add visit_function_param helper function
  • rib
  • toplevel
  • ctx: Add labels foreverstack
  • forever-stack: Fix basic get logic
  • late
  • foreverstack: Specialize get for Namespace::Labels
  • forever stack: Fix resolve_path signature
  • late: resolve path
  • early: resolve path
  • late: Add test
  • nameresolutionctx: Store mappings in ctx
  • early: [f | d]: Add macro invocations to mappings
  • late: Start setting up builtin types
  • late: builtins
  • late: Start storing mappings properly in the resolver
  • early
  • nameresolutionctx: Keep map of resolved usages
  • toplevel: Add comment about running the collector twice
  • ast: Add NodeId to UseTree base class
  • default resolver: Visit UseDeclarations
  • forever stack: Do not copy segment when dereferencing iterator in find_starting_point [other functions too?]
  • early: Move usedec resolving to TopLevel
  • toplevel: Start usedec resolution
  • default: [f] it to [d] the other
  • toplevel: Handle use declarations properly
  • session-manager: [f] STart dumping name reoslution pass
  • toplevel: Resolve use decls more properly
  • toplevel: Start cleanup of import handling
  • add more name res test casaes
  • immutable nrctx
  • session manager: Init Immutable name resolver
  • nrctx: add resolved lookup
  • typecheck: Fetch ImmutableNRCtx
  • foreverstack: Add to_canonical_path method
  • foreverstack: Add to_rib method
  • typecheck: Start using nr2.0 properly
  • backend: Use new name resolver where necessary
  • nr2.0: Start using newtype pattern for Usage and Definition
  • rib: Add base for storing shadowable ids
  • typecheck path: wip
  • late: Setup builtin types properly
  • rib: Add shadowable definitions on a per definition level
  • forever stack: Use new Rib::Definition API
  • toplevel: Add note about resolving glob imports
  • default: Visit external functions properly
  • compile: use new rib API
  • type-check-type: Smol cleanup in format string

DefaultResolver::visit (let);

// how do we deal with the fact that `let a = blipbloup` should look for a
// label and cannot go through function ribs, but `let a = blipbloup()` can?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let a = blipbloup can look through function ribs if blipbloup is a function, constant or static item.

@@ -54,6 +63,11 @@ TopLevel::insert_or_error_out (const Identifier &identifier, const T &node,
void
TopLevel::go (AST::Crate &crate)
{
// we do not include builtin types in the top-level definition collector, as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use i32; is allowed for editions >= 2018

auto def_fn = [this, &function] () {
for (auto &param : function.get_function_params ())
{
// TODO: So extern function params are not patterns?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rustc will parse any pattern, but it's an error if the pattern is anything other than an identifier or _ after expansion.

// so what is the plan for glob imports
// 1. figure out the rib which contains all the exports, in each namespace
// 2. for that namespace, go through all the nodes
// 3. filter out the exported ones (is that correct?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be all nodes that are visible (not private) from the import, even other imports. You do need to handle cycles somehow.


auto candidate = nr_ctx.values.to_rib (body_mappings.get_nodeid ());

// FIXME: Ugly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@CohenArthur CohenArthur force-pushed the arthur/dev branch 3 times, most recently from 177a4b3 to 5cd813b Compare September 14, 2023 16:14
@P-E-P
Copy link
Member

P-E-P commented Sep 19, 2023

optional: Add [[nodiscard]]

Isn't this C++17 ? Won't it break with 4.8 ?

@CohenArthur
Copy link
Member Author

optional: Add [[nodiscard]]

Isn't this C++17 ? Won't it break with 4.8 ?

yeah, this does break it. I'll remove the commits. it's very useful for testing however and works in C++11, the compiler just complains that this attribute is in C++17 and still applies it

@CohenArthur
Copy link
Member Author

@matthewjasper I opened issues for all your comments, but I don't think I'll be addressing them in this PR as the goal for this one is to not add any regressions, and thus it can be in an incomplete state. thanks a lot for having taken the time to review it :)

@CohenArthur CohenArthur force-pushed the arthur/dev branch 11 times, most recently from a79741a to 83b9bac Compare November 14, 2023 20:17
@CohenArthur CohenArthur force-pushed the arthur/dev branch 2 times, most recently from 01755c2 to 6a5265e Compare November 15, 2023 10:02
CohenArthur and others added 26 commits December 14, 2023 17:59
The resolver did report duplicate symbols when being run multiple times
even if the node id was the same. This commit adds an additional
condition, the error will only be reported if the existing node id is
different from the current node id.

gcc/rust/ChangeLog:

	* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::insert_or_error_out):
	Add new constraint to duplicate errors.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
Change the test's name resolution algorithm to 2.0.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution12.rs: Add name resolution 2.0 flag.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
Change test name resolution algorithm to 2.0.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution13.rs: Add compile flag to use the new
	name resolution 2.0 algorithm.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
The compiler did not emit any warning when a same target was declared
from different sources.

gcc/rust/ChangeLog:

	* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::handle_use_dec):
	Use the new dict to track down already resolved use declarations.
	* resolve/rust-toplevel-name-resolver-2.0.h: Add new dict to store
	previous use declarations.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
The original error text did not match the test nor rustc's behavior.

gcc/rust/ChangeLog:

	* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): Update
	error text to match rustc's one.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution14.rs: Update the test with error code.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
Change the name resoltion algorithm to 2.0 in this test.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution14.rs: Add flag to activate name
	resolution 2.0.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
Change name resolution algorithm in name resolution test 18 to the new
name resolution 2.0.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution17.rs: Add flag to use new name
	resolution algorithm.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
Change the name resolution algorithm to name resolution 2.0 in one name
resolution test.

gcc/testsuite/ChangeLog:

	* rust/compile/name_resolution18.rs: Add flag to enable name resolution
	algorithm.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
The resolver was emitting an error when meeting the same symbol twice.
What is important here is the origin of the resolved symbol, we should
emit an error when the name is similar but the symbol isn't be not when
the resolver is simply meeting the exact same symbol.

gcc/rust/ChangeLog:

	* resolve/rust-toplevel-name-resolver-2.0.cc (insert_macros): Add
	constraint over the ast node id.
	(TopLevel::visit): Likewise.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Copy link
Member

P-E-P commented May 29, 2024

@CohenArthur Should we close this PR ?

@CohenArthur CohenArthur closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants