Be more specific in NameError message

[exterm]

Before: "expected file #{x_rb} to define constant X, but didn't" After: "expected file #{x_rb} to define constant ::X, but didn't"

As far as I understand we're always looking for a top level constant.

I've repeatedly seen people be confused about this error message and I hope this small change can make it a little clearer.

Consider a file app/models/register.rb that defines Registration::Register. The current error message would say that it expects the file to define Register, which it kind of does... within the Registration namespace. Better be explicit and say that we expect it to define ::Register.

(Alternatively we could say "expected file X to define constant Register at the top level")

Question

Should I add explicit test cases for implicit namespaces and/or collapsed directories?

[fxn]

I see the room for improvement in this message, thanks for the patch.

As far as I understand we're always looking for a top level constant.

Hmmm, I don't know what do you mean by that. In app/models/hotel/pricing.rb we are not looking for a top-level constant. The top-level constant Hotel is not expected to be defined in that file, that file should reopen the Hotel class that was defined either in hotel.rb or autovivified by Zeitwerk (implicit namespace).

That is why

module Hotel::Pricing
end

works, because in a project managed by Zeitwerk Hotel always exists before that file hotel/pricing.rb is reached.

It is not conventional to write constant paths with leading colons. When you read documentation, or talk with your peers, you don't write the ::Admin::UsersController controller, you just say the Admin::UsersController controller. Because if you don't say anything else, a constant path is assumed to start at the root namespace.

I think we could delete the word "constant":

expected app/models/register.rb to define Register
expected app/models/users/register.rb to define Users::Register

What do you think?

[exterm]

Thank you for your quick response Xavier!

Hmmm, I don't know what do you mean by that. In app/models/hotel/pricing.rb we are not looking for a top-level constant.

In this case I believe the error message would mention Hotel::Pricing which is a top level constant, isn't it? I've been confused about nested constant definitions before though, it wouldn't be the first time...

It is not conventional to write constant paths with leading colons.

It is / has been conventional in the groups I have been doing Rails development with. We had a strong push for being explicit about what's top level a few years ago at Shopify, and this is also reflected in packwerk.

Because if you don't say anything else, a constant path is assumed to start at the root namespace

This is entirely dependent on context. If I'm working mostly within a User namespace I may very well say Registration and mean User::Registration, and indeed that's what I would be able to do in code. Conversely, ::Registration would always refer to the top level constant no matter the nesting context.

I think we could delete the word "constant":

I think this is a possible change that makes the message more concise but it would not solve the problem I am trying to solve, where a file contains a statement class Register yet zeitwerk complains about the file not defining Register (because the definition is nested within a namespace in the file).

# app/models/register.rb

module Users
  class Register
  end
end

If you don't like the explicitly fully qualified constant reference, which I can sympathize with, I propose the alternative of

expected app/models/register.rb to define Register at top level
expected app/models/users/register.rb to define Users::Register at top level

[fxn]

I see what you mean, problem is that Users::Register is not a top-level constant.

Users is a top-level constant, Register is a constant defined in Users. Indeed, Users::Register is not even a constant, it is a constant path, or a compound constant reference. It is an expression made of constants: First you lookup Users, if all goes well, that returns a class or module object and then you lookup Register in it.

In Users::Register there are two constants.

I believe when I wrote that error message I did not want to say constant Register in Users and did not use the terminology quite precisely, but we should probably revise that.

expected app/models/register.rb to define Register at top level
expected app/models/users/register.rb to define Register in Users

I am not sure I am 100% convinced of these alternatives (the reason why the message made some compromises seeking a balance with simplicity). Just thinking aloud to help us think about it.

[exterm]

Not to get too philosophical here, but the name of something is an identifier it can be referenced by, no?

For example, my name is Philip... but my name is also, more specifically, Philip Mueller. This is independent of whether you look me up in a nested hash that has last name as the outer hash key, first name as the outer hash key, or just a flat hash that uses full name as the key.

I can certainly look up Users::Register by first looking up Users, and I believe that's how the interpreter would resolve it, but does that make a practical difference? The constant can be referred to as Register (in certain contexts), more specifically as Users::Register (in more contexts), or even more specifically by ::Users::Register (in any context).

Wouldn't that make ::Users::Register something like its full (fullest) name?

So I guess what I'm saying is that a constant path is also a name for a constant.

And so I still think my original proposal makes sense: The constant that is most specifically referred to as ::Users::Register is not defined in this file. I just think the part in italics can be omitted since I wouldn't refer to you as "the person who can be referred to as Xavier" either.

My favourite at this point is

expected app/models/register.rb to define ::Register
expected app/models/users/register.rb to define ::Users::Register

[anwagner-onemedical]

As the person who mentioned this issue to Philip today and make him think about it, I'll weigh in here.

I agree that "top-level" is probably not the right term to use, it's more of a "local top-level", but I don't know of a good name for that concept.

For this:

Wouldn't that make ::Users::Register something like its full (fullest) name?

I think something like "Fully Qualified Name" would make sense to a lot of people, though I'm not sure how to usefully include that in this error message

I do think that these errors are much clearer than the original:

expected app/models/register.rb to define ::Register
expected app/models/users/register.rb to define ::Users::Register

...though I think, ideally, an even better error in the second case would be something like:

expected app/models/users/register.rb to define ::Users::Register or for ::Users::Registration:Register to be defined in app/models/users/registration/register.rb

That might be a bit of a pipe dream, though.

[fxn]

Not to get too philosophical here, but the name of something is an identifier it can be referenced by, no?

No. In this case we are dealing with a formal system, so things are well-defined.

I believe you might have some misconceptions about constants, I have known quite a lot of senior Ruby programmers who are not familiar enough with Ruby constants. And one of the reasons for that is that this topic is complex and not well-documented.

In Zeitwerk and Rails autoloading this is a challenge for documenting things. You can't be too precise or else the docs would not be didactic enough. For example, the README says

lib/my_gem.rb         -> MyGem
lib/my_gem/foo.rb     -> MyGem::Foo
lib/my_gem/bar_baz.rb -> MyGem::BarBaz
lib/my_gem/woo/zoo.rb -> MyGem::Woo::Zoo

and that already communicates a lot without using technical jargon. (Note there are no leading colons, by the way.)

I am writing a short book about this topic with the hope that at least there's a good reference somewhere.

The term "compound constant reference" is not mine, it is the terminology used by Matz himself. I normally don't use this term myself (but I will mention it in the book).

You totally have to understand that constants are stored in class and module objects:

module M
  C = 1
end

M.constants # => [:C]

See, the constant is :C, not M::C.

The name of the constants involved in the snippet above is "M" and "C". That is the way it is. The first one is stored in the class object stored in the Object constant, and the second one is stored in the module object stored in the M constant. (Being pedantic to unroll everything so that the concepts become clear, you do not talk that way in the day to day.)

The names of constants have no colons.

Why does Matz use the term "reference"? Because you are referencing an entity stored in a class or module object. For example:

m = Module.new
m::C = 1

X = m
Y = m
Y::Z = m

X::C    # => 1
Y::C    # => 1
Y::Z::C # => 1

X::C, Y::C, Y::Z::C are references, they are constant paths that end up resolving to the constant called "C" stored in the module object stored in the variable m.

Both Admin and Admin::UsersController are constant paths. Every segment in a constant path is a constant that is looked up in the class or module object the expression to the left of it evaluates to. That lookup also checks ancestors if necessary, etc., the path does not mean all constants are stored exactly in the class or module objects being resolved as you walk the constant path from left to right.

OK, it is late here. Let me sleep on this issue.

[fxn]

I believe the main pain point here is top-level constants, because it can be argued that

expected file #{x_rb} to define constant X, but didn't

is ambiguous. It is, indeed, because as I said yesterday M::X = 1 is defining a X constant. And that is the confusion you guys have seen in other people.

I have to say that Zeitwerk is 5 years old and this is the first time I hear about this confusion. It has to happen that you define a supposedly top-level constant within a namespace, and do not get the simple naming conventions that map file paths to constant paths to interpret the error message. But, nonetheless, I agree the message is technically ambiguous and can be improved.

The question is how. I have pondered several options:

1. "expected ... to define constant ::X"
2. "expected ... to define a constant X in the top-level namespace"
3. "expected ... to define the top-level constant X"

You need to consider how does that translate to namespaces, to have a consistent style:

1. "expected ... to define constant ::M::X"
2. "expected ... to define a constant X in the M namespace"
3. "expected ... to define the constant M::X"

While option (1) is technically correct, that is not conventional notation (open any API, books, guides, NameError exception messages, etc., a constant path is never prefixed with :: unless necessary). Outside of source code, a constant path is always assumed to start at the root namespace. If I introduce that, in addition to being a bit unsual, I can imagine creating an expectation in junior devs that you are supposed to write the colons when you fix the file. My gut feeling says the proposal, while correct, does not quite feel right to me.

Option (2) is more accurate, but also more verbose. I suspect that can be confusing specially when you just made a mistake or need an inflection, which are the common use cases you have to design for. Compare "expected ... to define MyParser::Xml" to "expected ... to define a constant Xml in the MyParser namespace". If all you need to realize is that "XML" needs an inflection, I find the former to be more clear than the latter.

For me, option (3) is that one that strikes the best balance. It removes the ambiguity for top-level constants and keeps the message simple and idiomatic both for top-level and namespaced constants.

So, I have implemented (3) in https://github.com/fxn/zeitwerk/commit/167122946228fa6b1f53ea933c5bcd12a28facc3.

(I have removed also the trailing ", but didn't", with time I have found that to be too redundant.)

[exterm]

Thank you Xavier, I believe your change does indeed make the error message less confusing.

The argument that convinces me is that "expected ... to define constant ::X" could confuse junior devs into writing code like module ::X, which could cause further confusion down the road. So I do think the solution you arrived at is a good middle ground.

Further notes for posterity (no response required, unless you would like to continue the discussion):

No. In this case we are dealing with a formal system, so things are well-defined.

I understand that you're disagreeing with the notion that a constant path is a name for a constant.

I still believe that one and the same constant can have multiple names (your example demonstrates multiple constant paths that refer to the same constant), and we're choosing the most relevant of them for the error message based on zeitwerk's conventions for how constant paths map to file paths as well as the (useful, IMO) assumption that any reference not relative to the top level namespace would be confusing.

I also still think the notation with a leading :: is useful to make it explicit that a constant reference is intended to resolve in the top level namespace. In Ruby code, there is no other way to express "A::B in the top-level namespace".

[fxn]

You don't discuss over a drink what is an even integer, or debate if 0 is even or not. That is already defined, you open a math book and look it up.

Similarly, with constants in Ruby, this is not up for discussion. The name of a constant does not have colons. They are simple identifiers which become keys in a class or module constants table.

Ruby provides different ways to access the value stored in a constant stored in a certain class or module object. There are constant paths, there is relative lookup, there's the constants API, etc.

[fxn]

In an error message, you don't print just the name. You need to tell the user in which class or module was the constant missed, and in order to do that you concatenate the class or module name.

Note that the resulting constant path may not even work if one of the constants in the class or module name was removed. But that does not matter, because you are not giving a working constant path, you are telling the user in which class or module was the constant (simple name) missing.

Similarly, in const_missing you receive the name of the missing constant (simple) in the target class or module object (the receiver).

[fxn]

Of course, the target class or module may be anonymous too, in which case the error message prints something that is not even a constant path.

Name of constant is simple is a fact. We could leave it here, but I am giving you different ways to look at it so you can separate constant name from constant access from informative error message.

[fxn]

Heads up: after giving it some time, I have switched to option (2):

expected ... to define X in the top-level namespace
expected ... to define Y in the namespace A::B

The rationale is in the commit message (https://github.com/fxn/zeitwerk/commit/3d43c2fc2a85894d899a14b7c852279b9d2ef127).

I am playing with the idea, can't promise any of this will actually ship. Maybe it does, maybe it does not, I need to decide.