wit-bindgen icon indicating copy to clipboard operation
wit-bindgen copied to clipboard

Disallow duplicate names between types and functions

Open esoterra opened this issue 4 years ago • 3 comments

Implementation for #162 it makes the duplicate identifier checking more general. A few tests have duplication that had to be resolved, I chose to do so by appending -func to functions which collide with types.

Open questions:

  1. Is the duplication logic in define_resource and define_type necessary if we have general checking at the level implemented in this PR?
  2. Is there a more proper type to use instead of the inline enum Definition I introduce for tracking what a prior name use was for? Alternatively, would it make more sense to just use &str?

esoterra avatar Mar 10 '22 03:03 esoterra

I don't seem to have a reply-in-line box so I'll reply out here to https://github.com/bytecodealliance/wit-bindgen/pull/169#discussion_r826109880. I think general architectural things are fine to refactor here, I'm mostly trying to avoid lots of levels of duplicate hash maps since that can be a bit confusing. If you see other things to improve I think that would work well too.

alexcrichton avatar Mar 15 '22 14:03 alexcrichton

In implementing @alexcrichton's suggestion to unify the resources, types, functions, and globals into one namespace that handles duplicate names, I'm running into some places where that doesn't fit cleanly in the existing model.

  1. There's a conflict between resources and their handles living in the same namespace but being distinct things.
  2. In different syntactic contexts e.g. foo: handle <name> vs. foo: <name> (which are represented as different AST nodes) only types or resources are actually valid, so in that sense they aren't even colliding.

These conflicts make me want to either

  • remove the handle keyword and treat resources as a form of type, or
  • only unify the namespaces for types, globals, functions but not resources.

esoterra avatar Jul 01 '22 17:07 esoterra

We talked briefly on Zulip, but I think it's fine to (a) either leave the resource namespace for now and land this or (b) remove handle foo in favor of foo and put everything into the same namespace.

alexcrichton avatar Jul 01 '22 19:07 alexcrichton

This is pretty old now but this was added in #457 so I'm going to close this now.

alexcrichton avatar Jan 26 '23 15:01 alexcrichton