Add option_env support #3094

liamnaddell · 2024-07-22T00:50:32Z


    gcc/rust/ChangeLog:
            * expand/rust-macro-builtins-utility.cc: Add macro expansion for
            option_env with eager expansion
            * expand/rust-macro-builtins.cc: Add option_env to builtin list
            * expand/rust-macro-builtins.h: Add option_env handler to header
            file

    gcc/testsuite/ChangeLog:
            * rust/compile/macros/builtin/option_env1.rs: Add success case for option_env
            * rust/compile/macros/builtin/option_env2.rs: Add failure case for option_env
            * rust/execute/torture/builtin_macro_option_env.rs: Add
            execution case for option_env
            * rust/compile/nr2/exclude: Some issues with nr2 need to be
            resolved before allowing option_env with NR2.0.

Fixes #1806

Addresses #927, #1791

Here is a checklist to help you with your PR.

GCC development requires copyright assignment or the Developer's Certificate of Origin sign-off, see https://gcc.gnu.org/contribute.html or https://gcc.gnu.org/dco.html
Read contributing guidlines
make check-rust passes locally
Run clang-format
Added any relevant test cases to gcc/testsuite/rust/

Adds compiler support for option_env, adds eager expansion for option_env, adds tests for option_env

CohenArthur

honestly, well done - this isn't an easy part of the codebase, and you did a really good job :) don't worry about the tokenstream stuff, we can think about fixing this later. for the path issue, you're hitting a very frustrating issue which I'm trying to fix in #3068. ideally, we should be able to create paths to lang items (such as Option::Some and Option::None) so that we don't care about the module structure, or the name, or anything - we just care about hitting the proper type from the standard library. good work!

gcc/rust/expand/rust-macro-builtins-utility.cc

gcc/testsuite/rust/compile/option_env1.rs

liamnaddell · 2024-07-24T22:13:19Z

honestly, well done - this isn't an easy part of the codebase, and you did a really good job :)

Thank you :)

liamnaddell · 2024-08-20T12:52:06Z

@CohenArthur Should I update this PR to use the lang-item paths from #3068 ?

P-E-P · 2024-09-10T11:20:28Z

@CohenArthur Should I update this PR to use the lang-item paths from #3068 ?

I've been trying to catch up on that issue (sorry for the delay!). We should probably rely on lang item path because right now the tests make use of a core module where it should be the core library (aka technically the file itself since we're defining the builtin macro from within the crate).

liamnaddell · 2024-09-10T15:27:58Z

@CohenArthur Should I update this PR to use the lang-item paths from #3068 ?

I've been trying to catch up on that issue (sorry for the delay!). We should probably rely on lang item path because right now the tests make use of a core module where it should be the core library (aka technically the file itself since we're defining the builtin macro from within the crate).

Ok, I can wait until the lang items path stuff is usable. If appropriate, we can also revisit some of the other macros then as well to integrate lang items.

liamnaddell · 2024-09-10T15:28:41Z

Converting to a Draft PR until we have lang items integrated.

CohenArthur · 2025-01-17T16:34:24Z

gcc/rust/ast/rust-collect-lang-items.cc

+  if (tl::optional<LangItem::Kind> lang_item = get_lang_item_attr (item))
+    {
+      rust_debug ("[CollectLangItems] Adding lang item %s",
+		  LangItem::ToString (*lang_item).c_str ());
+      mappings.insert_lang_item_node (lang_item.value (), item.get_node_id ());
+    }


I'd say remove this, but feel free to keep it if you think it helps

CohenArthur · 2025-01-17T16:59:23Z

gcc/rust/ast/rust-path.h

-    rust_assert (kind == Kind::Regular);
+    // For lang-items, this returns an empty path.
    return segments;


I think we should keep the original here. Everywhere that calls .get_segments() where a lang item can also be present need to be handled differently to resolve to the proper lang item anyway, so the assertion makes sense

Ok, I can change this, I think it will require adding some code like:

if (path.is_lang_item()) return;

to various AST passes, since a lot of these passes try to recurse into a PathInExpression's segments

CohenArthur · 2025-01-17T16:59:31Z

gcc/rust/ast/rust-path.h

-    rust_assert (kind == Kind::Regular);
+    // For lang-items, this returns an empty path.


CohenArthur · 2025-01-17T17:04:16Z

gcc/rust/hir/tree/rust-hir-path.h

 		    std::vector<PathExprSegment> path_segments,
+		    tl::optional<LangItem::Kind> lang_item = tl::nullopt,


I'd like to see two constructors, one with segments and one with the lang item. Is that code I wrote or is it something you added for this PR?

I added all lang item handling at/below HIR level for PathInExpression.

I can fix this, this was a hack I forgot to remove. I'll try to handle HIR::PathInExpression similarly to AST::PathInExpression as far as it relates to constructors

CohenArthur · 2025-01-17T17:04:53Z

gcc/rust/hir/rust-ast-lower.cc

+	= new HIR::PathInExpression (mapping, std::move (path_segments),
+				     expr.get_lang_item (), expr.get_locus (),


so we'd get rid of the segments here for example

Suggested change

= new HIR::PathInExpression (mapping, std::move (path_segments),

expr.get_lang_item (), expr.get_locus (),

= new HIR::PathInExpression (mappin, expr.get_lang_item (), expr.get_locus (),

CohenArthur · 2025-01-17T17:31:47Z

gcc/rust/typecheck/rust-hir-type-check-path.cc

+void
+TypeCheckExpr::handle_enum_lang_item (HIR::PathInExpression &expr)
+{
+  // Find the lang item's definition
+  Analysis::Mappings &mappings = Analysis::Mappings::get ();
+  NodeId lang_item_node = mappings.get_lang_item_node (expr.get_lang_item ());
+  tl::optional<HirId> ohid = mappings.lookup_node_to_hir (lang_item_node);
+  rust_assert (ohid != tl::nullopt);
+  HirId hid1 = *ohid;
+  std::pair<HIR::Enum *, HIR::EnumItem *> ohi
+    = mappings.lookup_hir_enumitem (hid1);
+
+  HIR::Enum &enum_def = *(ohi.first);
+  HIR::EnumItem &variant_def = *(ohi.second);
+
+  TyTy::BaseType *bl = TypeCheckItem::Resolve (enum_def);
+  TyTy::VariantDef *vde
+    = TypeCheckEnumItem::Resolve (variant_def, INT64_MAX - 1);
+
+  context->insert_variant_definition (expr.get_mappings ().get_hirid (),
+				      vde->get_id ());
+  bl = SubstMapper::InferSubst (bl, expr.get_locus ());
+  resolver->insert_resolved_misc (expr.get_mappings ().get_nodeid (),
+				  expr.get_mappings ().get_nodeid ());
+
+  infered = bl;
+  rust_assert (bl != nullptr);
+  return;
+}


@philberty how does that look? you know the typechecker better than me lol

As a note, I'm looking at this code again, and it seems like some of the assert's might want to be compile errors instead maybe. I'm thinking of the case where somebody puts #[lang = 'Some'] on something that's not an enum variant

that's fine, because having "wrong lang items" is such a corner case that we don't really need to handle it. we know that core is correct code, if someone wants to reimplement their own core then the burden falls on them rather than on the compiler. asserts are better IMO

liamnaddell · 2025-01-17T17:39:15Z

Also I see the build-and-check with glibcxx assertions turned on ICE's. I'll try to figure out what's going on there. (gcc is giving me trouble with compiling :/ )

CohenArthur · 2025-01-17T17:59:48Z

Also I see the build-and-check with glibcxx assertions turned on ICE's. I'll try to figure out what's going on there. (gcc is giving me trouble with compiling :/ )

might be because we're doing something like .get_segment().front() somewhere, which triggers the assertions (and is UB). this is also why I think we should separate lang items and segmented paths and always make get_segments() assert that we're dealing with a segmented path

liamnaddell · 2025-01-17T21:33:59Z

Also I see the build-and-check with glibcxx assertions turned on ICE's. I'll try to figure out what's going on there. (gcc is giving me trouble with compiling :/ )

might be because we're doing something like .get_segment().front() somewhere, which triggers the assertions (and is UB). this is also why I think we should separate lang items and segmented paths and always make get_segments() assert that we're dealing with a segmented path

Because of my env, I was compiling gccrs with gccrs, which didn't work, because I think gccrs' gcc wasn't properly configured. I changed to use system gcc and it compiles perfectly.

Your guess was exactly what it was. the ICE was rust-hir-path.cc:266 segments.back ()

liamnaddell · 2025-01-18T02:57:52Z

gcc/rust/backend/rust-compile-resolve-path.cc

+      TyTy::BaseType *lookup = nullptr;
+      bool ok
+	= ctx->get_tyctx ()->lookup_type (expr.get_mappings ().get_hirid (),
+					  &lookup);
+      rust_assert (ok);
+
+      tree t = attempt_constructor_expression_lookup (lookup, ctx,
+						      expr.get_mappings (),
+						      expr.get_locus ());
+      TREE_USED (t) = 1;
+      resolved = t;


@CohenArthur That GLIBCXX assert CI/CD job caught some undefined behavior, where I pushed a random address into the ResolvePathRef::resolve function as the "final segment". I added code here to properly handle lang-item paths.

(Still working on fixing the AST::PathInExpression, I should be able to get that done soon)

CohenArthur

that's great work, looks perfect to me :D thank you so much @liamnaddell !

CohenArthur · 2025-01-31T11:54:37Z

gcc/rust/resolve/rust-late-name-resolver-2.0.cc

+  if (expr.is_lang_item ())
+    return;


I think resolution here should be pretty easy but we'll do this in another PR - I'm happy with that fix

gcc/rust/ChangeLog: * expand/rust-macro-builtins-utility.cc: Add macro expansion for option_env with eager expansion * expand/rust-macro-builtins.cc: Add option_env to builtin list * expand/rust-macro-builtins.h: Add option_env handler to header file * resolve/rust-late-name-resolver-2.0.cc: Prevent NR2.0 from recursing into lang-item segments gcc/testsuite/ChangeLog: * rust/compile/macros/builtin/option_env1.rs: Add success case for option_env * rust/compile/macros/builtin/option_env2.rs: Add failure case for option_env * rust/execute/torture/builtin_macro_option_env.rs: Add execution case for option_env

liamnaddell marked this pull request as draft July 22, 2024 00:50

liamnaddell force-pushed the option_env branch from 1fc99ec to 21c43ec Compare July 23, 2024 00:07

liamnaddell marked this pull request as ready for review July 23, 2024 00:07

liamnaddell force-pushed the option_env branch 3 times, most recently from 6262158 to 62a89ef Compare July 23, 2024 02:12

CohenArthur reviewed Jul 24, 2024

View reviewed changes

gcc/rust/expand/rust-macro-builtins-utility.cc Outdated Show resolved Hide resolved

gcc/testsuite/rust/compile/option_env1.rs Outdated Show resolved Hide resolved

liamnaddell force-pushed the option_env branch from 62a89ef to 20d7b4b Compare July 24, 2024 22:03

liamnaddell force-pushed the option_env branch 6 times, most recently from 8350b2c to dd8612c Compare August 1, 2024 22:54

liamnaddell force-pushed the option_env branch 2 times, most recently from 36cd2a3 to 2034102 Compare August 8, 2024 01:03

liamnaddell force-pushed the option_env branch 2 times, most recently from 74ad9b9 to 1f41fa7 Compare August 15, 2024 22:01

liamnaddell force-pushed the option_env branch from 1f41fa7 to 3f39ce6 Compare August 20, 2024 22:50

liamnaddell requested a review from CohenArthur August 27, 2024 22:34

liamnaddell force-pushed the option_env branch 3 times, most recently from ca99a70 to 962dc4d Compare September 2, 2024 19:57

liamnaddell marked this pull request as draft September 10, 2024 15:28

liamnaddell force-pushed the option_env branch from 962dc4d to 6ca4eae Compare December 15, 2024 22:52

liamnaddell force-pushed the option_env branch 8 times, most recently from ef93339 to 1596237 Compare January 17, 2025 14:53

CohenArthur reviewed Jan 17, 2025

View reviewed changes

liamnaddell force-pushed the option_env branch from 1596237 to 8e8fdcc Compare January 18, 2025 02:54

liamnaddell commented Jan 18, 2025

View reviewed changes

liamnaddell force-pushed the option_env branch 3 times, most recently from a8b5ef0 to 455fff0 Compare January 22, 2025 04:08

liamnaddell force-pushed the option_env branch 5 times, most recently from 333d52d to f50cdf6 Compare January 31, 2025 02:55

CohenArthur approved these changes Jan 31, 2025

View reviewed changes

CohenArthur mentioned this pull request Jan 31, 2025

Resolve lang-item PathInExpressions in nr2.0 late resolver #3404

Open

liamnaddell force-pushed the option_env branch from f50cdf6 to 705c3f6 Compare January 31, 2025 15:19

CohenArthur enabled auto-merge January 31, 2025 15:28

CohenArthur added this pull request to the merge queue Jan 31, 2025

Merged via the queue into Rust-GCC:master with commit 761d424 Jan 31, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option_env support #3094

Add option_env support #3094

liamnaddell commented Jul 22, 2024 •

edited

Loading

CohenArthur left a comment

liamnaddell commented Jul 24, 2024

liamnaddell commented Aug 20, 2024

P-E-P commented Sep 10, 2024

liamnaddell commented Sep 10, 2024

liamnaddell commented Sep 10, 2024

CohenArthur Jan 17, 2025

CohenArthur Jan 17, 2025

liamnaddell Jan 17, 2025 •

edited

Loading

liamnaddell Jan 19, 2025

CohenArthur Jan 17, 2025

liamnaddell Jan 19, 2025

CohenArthur Jan 17, 2025

liamnaddell Jan 17, 2025 •

edited

Loading

liamnaddell Jan 19, 2025

CohenArthur Jan 17, 2025

liamnaddell Jan 19, 2025

CohenArthur Jan 17, 2025

liamnaddell Jan 17, 2025

CohenArthur Jan 31, 2025

liamnaddell commented Jan 17, 2025 •

edited

Loading

CohenArthur commented Jan 17, 2025

liamnaddell commented Jan 17, 2025

liamnaddell Jan 18, 2025

CohenArthur left a comment

CohenArthur Jan 31, 2025

		rust_assert (kind == Kind::Regular);
		// For lang-items, this returns an empty path.

		std::vector<PathExprSegment> path_segments,
		tl::optional<LangItem::Kind> lang_item = tl::nullopt,

		= new HIR::PathInExpression (mapping, std::move (path_segments),
		expr.get_lang_item (), expr.get_locus (),

	= new HIR::PathInExpression (mapping, std::move (path_segments),
	expr.get_lang_item (), expr.get_locus (),
	= new HIR::PathInExpression (mappin, expr.get_lang_item (), expr.get_locus (),

Add option_env support #3094

Add option_env support #3094

Conversation

liamnaddell commented Jul 22, 2024 • edited Loading

CohenArthur left a comment

Choose a reason for hiding this comment

liamnaddell commented Jul 24, 2024

liamnaddell commented Aug 20, 2024

P-E-P commented Sep 10, 2024

liamnaddell commented Sep 10, 2024

liamnaddell commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamnaddell Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamnaddell Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamnaddell commented Jan 17, 2025 • edited Loading

CohenArthur commented Jan 17, 2025

liamnaddell commented Jan 17, 2025

Choose a reason for hiding this comment

CohenArthur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamnaddell commented Jul 22, 2024 •

edited

Loading

liamnaddell Jan 17, 2025 •

edited

Loading

liamnaddell Jan 17, 2025 •

edited

Loading

liamnaddell commented Jan 17, 2025 •

edited

Loading