cfg in file modules is not handled correctly

[matthewjasper]

// a.rs
#[path = "b.rs"]
mod module;

// b.rs
#[cfg(a)]
mod submod {}

#[cfg(not(a))]
mod submod {}

I expected to see this happen: compiles

Instead, this happened: error

./b.rs:5:1: error: redefined multiple times
    2 | mod submod {}
      | ~~~
......
    5 | mod submod {}
      | ^~~

Meta

• What version of Rust GCC were you using, git sha if possible. 3d2a0c0445c2a1417dcc290b7a14e551c14fa421

[philberty]

we are probably missing a check in the toplevel name resolution for module is_marked_for_strip

[CohenArthur]

I wish our cfg stripping pass simply returned a new AST without all the "to strip" nodes haha. it would be so much nicer to use, we wouldn't have to check for is_marked_for_strip anywhere

[matthewjasper]

I think that the cfg visitor is not even processing these items.

[CohenArthur]

void
CfgStrip::visit (AST::Module &module)
{
  // strip test based on outer attrs
  expand_cfg_attrs (module.get_outer_attrs ());
  if (fails_cfg_with_expand (module.get_outer_attrs ()))
    {
      module.mark_for_strip ();
      return;
    }

  // A loaded module might have inner attributes
  if (module.get_kind () == AST::Module::ModuleKind::LOADED)
    {
      // strip test based on inner attrs
      expand_cfg_attrs (module.get_inner_attrs ());
      if (fails_cfg_with_expand (module.get_inner_attrs ()))
	{
	  module.mark_for_strip ();
	  return;
	}
    }

  // strip items if required
  maybe_strip_pointer_allow_strip (module.get_items ());
}

it should, but as @philberty said, we do not check if modules are marked for strip or not in the name resolver:

  void visit (AST::Module &module) override
  {
    auto mod
      = CanonicalPath::new_seg (module.get_node_id (), module.get_name ());
    auto path = prefix.append (mod);
    auto cpath = canonical_prefix.append (mod);

    resolver->get_name_scope ().insert (
      path, module.get_node_id (), module.get_locus (), false,
      Rib::ItemType::Module,
      [&] (const CanonicalPath &, NodeId, Location locus) -> void {
	RichLocation r (module.get_locus ());
	r.add_range (locus);
	rust_error_at (r, "redefined multiple times");
      });

    NodeId current_module = resolver->peek_current_module_scope ();
    mappings->insert_module_child_item (current_module, mod);
    mappings->insert_module_child (current_module, module.get_node_id ());

    resolver->push_new_module_scope (module.get_node_id ());
    for (auto &item : module.get_items ())
      ResolveTopLevel::go (item.get (), path, cpath);

    resolver->pop_module_scope ();

    mappings->insert_canonical_path (module.get_node_id (), cpath);
  }

[CohenArthur]

we can probably fix this bug as part of the name resolution rework