Beginner friendly 'require' message when a constant is not found, but the constant is in the codebase but not required yet

[paulcsmith]

Relates to #5291 I started this new issue because the other one was already quite long.

# src/send_email/invitation_email.cr
class InvitationEmail
  include Mailer
  # ...code
end

# src/send_email/mailer.cr
module Mailer
  # ...code
end

# src/send_email.cr
require "./emails/*
InvitationEmail.new.send

This will fail to find the constant Mailer when you run crystal src/send_email.cr because the files are loaded alphabetically. Since InvitationEmail is loaded before the required Mailer dependency it fails. This may be unexpected for newcomers if they are using require "*" and it works fine until they happen to have a dependency that due naming is not loaded yet.

I think it would be helpful for the compiler to check the src folder for unrequired files that have a class/module that matches and tell the user what to require to get it working. In this case:

Could not find module 'Mailer' in 'src/send_email/invitiation_email.cr'

Maybe you need to require it first? 

   # in src/send_email/invitation_email.cr
   require "./mailer"

It wouldn't find everything (for example if you forgot to load "colorize" and tried to use the colorize method), but I think this would help a lot for some of the common cases.

Note, I am not saying the compiler actually compiles the unrequired file, it just does a simple text search looking for the constant in .cr files so it can see if you forgot to require one.

Challenges

Constants nested in modules like this would be easy to find:

class MyNamespace::MyClass
end

But I imagine this would be harder

module MyNameSpace
  class MyClass
  end
end

Different solutions?

I'm not sure this is the best solution, but this is definitely a problem I've seen a number of times that trips people up. I'd love to hear alternatives if this won't work or would be too hard

From #6122 (comment)

A super simple fix would be to mention in the Constant not found message that requires are done in alphabetical order so you may need to explicitly require the file. That alone might be enough!

[jwoertink]

We actually ran in to this issue. I wasn't aware that the files got loaded like that. I assumed that since it's all compiled anyway, LLVM just knows how to map where everything is. I feel like this should be built in to crystal itself. But I guess until that happens, having something helpful here would be pretty cool.

edit: just realized this is on the crystal repo and not lucky lol.

[paulcsmith]

Thanks for chiming in with your experience @jwoertink. I think the problem with that is that objects can be monkey patched and sometimes depend on when they're required. For example, you wouldn't want to accidentally load the monkey patched class before the original, but Crystal wouldn't know which one is which. It would be cool if it could be done, but I'm not sure it is possible/desirable because of the potential confusion around ordering. I'd love to be proven wrong though because that would be awesome. Something like autoload from Rails (but not so mystical)

[asterite]

Did you ever use Ruby? It's exactly the same.

[asterite]

To fix this, just explicitly require the types you need before including/extending them.

[paulcsmith]

I think it is fairly common for people to require whole folders and have it work and then suddenly stop working when they add a file that earlier in the alphabet. This can be confusing because it looks like it should be loaded.

Also, a lot of people using Ruby have almost exclusively used Rails, where you often never write any requires other than "rails_helper". This being the case, I think it would be nice for Crystal to help you out. It's helpful for beginners and non-beginners. Maybe you just forgot to require it or aren't sure where it is, now Crystal will help push you in the right direction. A lot of things that seem obvious are not so obvious at first, this is one of them

[jwoertink]

Yeah, in Ruby, I tend to have a file called "all_the_things.rb" and then I do require "alll_the_things" just so I know what order things are being required in. I do the same in Crystal when I run in to this issue (which isn't often), but it would be nice to have an error message the helped point me in the direction that first time.

An easy fix here might just be adding some extra info here saying that the files are required in alphabetical order, and use an example of when that could cause an issue.

[paulcsmith]

I wanted to see if this was just me doing it, but it is likely most library authors. If you run crystal init lib <my-lib> the default generated files use an asterisk to load all files in the folder require "<my-lib>/*"

Most every Crystal lib does this and can be bitten by it if you don't know what it is doing. A helpful message would be nice.

A super simple fix would be to mention in the Constant not found message that requires are done in alphabetical order so you may need to explicitly require the file. That alone might be enough to point people in the right direction. Otherwise there will almost certainly be some confusion for new people

[paulcsmith]

@oprypin Can you explain why you gave it a thumbs down?

[paulcsmith]

I also think it would be helpful to be aware of https://en.wikipedia.org/wiki/Curse_of_knowledge

It seems so simple, but for new people, this can be a problem. At the very least something in the error message letting them know that the require is alphabetical might help them find the problem. Otherwise people think it is broken

[oprypin]

The problem is using those wildcards in the first place. Other languages either don't have them or ban them in style guides.

Adding horrible hacks on top of horrible hacks doesn't sound like a good idea.

[paulcsmith]

@oprypin That is understandable. If wildcard require is not preferred, then it should be changed in generated crystal projects because that's what every lib starts out with: #6122 (comment)

I also think that if you can't require a folder, you should be able to require from the project root, otherwise you get into a convoluted mess of relative requires. But this is likely better in a separate discussion.

Personally, I like the wildcard require and am fine with requiring a file here and there when needed, it's just not clear when you first run into it why it isn't working. Once you know it's fairly simple.

In the meantime, I think we could go with the simpler solution of mentioning the require is alphabetical if it can't find a constant and you're using a wildcard. It seems that would be fairly simple change and would resolve most of the issues. Thoughts?

[asterite]

Maybe we should remove wildcards from require... I think they are more bothersome than useful.

[oprypin]

I certainly support that but fear the community outcry

[paulcsmith]

I definitely like wildcard require and I think I'm not alone on that, but even if it is removed at some point, would adding the more helpful message about how things are required be difficult in the interim? If you could point me in the right direction I'd be happy to open a PR

Also worth noting that I've had far more problems with relative requires than with wildcard ones. Missing one level of /.., small typos, renaming files, moving files to a different folder, etc. Wildcard has been pretty painless once I knew about the alphabetical ordering

[asterite]

Wildcard is bad because of the reasons mentioned here, but also because if we ever want to implement an incremental compilation, the require "something/**" will have to check if any file is added in that directory, so that's just a bit harder. It's also not clear what gets required. And many times I wanted to require many files in a directory except one, but I already had *, so I had to change that to manually require each file. Requiring each file independently is not a big deal.

I feel this language is "let's type less" oriented, and nothing else... that's not a good philosophy for a language.

[oprypin]

You know what? The problem is not even the wildcards. Your file actually depends on every file in that directory? Sure, knock yourself out. But, that does not exempt you from declaring dependencies in those files. The solution to everything is very simple. You need to use something? You require it.

Surely, if you write a file without any requires you don't expect it to magically have all the needed dependencies. Why, then, is it a surprise that adding require * one level above does not magically solve this?

---

where you often never write any requires other than "rails_helper".

Disgusting

[asterite]

The solution to everything is very simple. You need to use something? You require it.

Yup, that's my suggestion too. If we want to make this strict, let's change require to a proper ìmport` system. Adding friendly error messages trying to guess what the user wanted to do feels like a workaround.

[oprypin]

The problem with an import system is that currently there is too much reliance on "re-exporting" files into a top-level file meant to be required. It would be incompatible with the current idiom of having many files implementing one module, and so require changes to almost everything.

Honestly, if the suggested warning worked well, it would be a good step towards discouraging wildcards as well. But there are many problems with the implementation, both false negatives and false positives. Wouldn't most codebases have a wildcard somewhere -- then how do you know that it's the wildcard in "your" code that caused the breakage?

[paulcsmith]

@oprypin your communication is extremely off-putting and does not follow the guidelines in https://github.com/crystal-lang/crystal/blob/master/CODE_OF_CONDUCT.md#our-standards. This is not the only issue this has happened. Please try to show empathy, understand where others are coming from (even if they are wrong/misinformed!) and use less abrasive language.

To address your point, whether it is disgusting or not, a large portion of people coming to Crystal is used to that. If requiring with a wildcard is in the language, I think having a hint for people is worth adding. If it's an easy change (just printing "requires are alphabetical", not saying what to require), it might be worth adding while we discuss whether to remove wildcard requires completely. If wildcard requires are removed then, of course, the message doesn't matter.

@asterite I don't think it is a less typing language, at least not in my viewpoint. For me, it is about focusing on what matters to my domain by removing things I need to know and focus on. Requiring things isn't necessarily helpful. In Elixir you do not ever need to require anything (with the exception of modules that have a macro). I realize that won't work in Crystal, but the point is that the language is conceptually simple for the programmer. You define a module, you use it. That's it. You can alias things and import them to make calling them shorter, but that is your choice. It isn't required.

I can see how removing wildcard can be simpler in some sense, but in its current implementation, it is not. It is tied too tightly to the structure of your folders, making it a pain to use. If you decide to move things to another subfolder, you have to change all your requires. You also have to use a different require based on where you're calling from. If wildcards are removed I think there should be something similar to webpack where you can set a resolver path so that tiny changes to directory structure aren't so painful. It also makes it easier to reference files because you use the same require in every file instead of changing it based on where the file currently is. Alternatively, having a wait to require from the working directory would be nice. Thoughts?

In closing, if requiring wildcards won't be removed for a few months (that would be my guess since it would require a lot of changes to a lot of libraries to have a smooth transition), then is it worth adding a simple message (just the note about alphabetical requires) to help people in the meantime? If not, then I think we should close this and open another issues about removing wildcard requires

[asterite]

I can't see a nice way to implement this. The compiler sees:

include Mailer

and it can't find it. Where is it defined? You know it's in a require "*" but the compiler doesn't. It could be anywhere, or maybe you did forgot to require it. Should the compiler parse all files and try to type them to find the missing class/module? I don't think that would work.

[paulcsmith]

@asterite What about the simpler version I suggested where it doesn't try to be too smart?

I image it would work something like this for the simpler version:

• If there is no close match (e.g., there is no "did you mean?")
• Then show the standard error message with some hints (like Elm does):

Undefined constant 'Mailer' in 'wherever'

Try this...

* Make sure you spelled it correctly
* If you are using a wildcard `require "/*`, you may need to require a file explicitly: require "./my_dependency"

# Exact language can be improved

These type of messages are in Lucky and have been received well. It doesn't try to be too smart, and it is just helpful enough to point people in the right direction

[paulcsmith]

And if there were the more complex version I originally suggested, I think it would be fairly naive:

• If there is no close match (e.g., there is no "did you mean?")
• Read each files in the directory where the current file(the one that failed to compile) lives, since that's when issues with alphabetization come up
  • Do a simple string search for the constant
  • If it matches, suggest `require "./file-with-constant-in-it"

Undefined constant 'Mailer' in 'wherever'

We found 'Mailer' in src/send_email/mailer.cr. If that is the file you want, require it:

   require "./mailer.cr"

# Exact language can be improved

No parsing the files or anything crazy. It might not find everything, but it doesn't have to. Just like did you mean? doesn't always work, but when it does, it is helpful

Note: I'd be happy to implement if given some brief pointers on where to hook into. I don't have much experience with the compiler

[faustinoaq]

@paulcsmith Nice issue! 😉

I think a simple message like you commented here #6122 (comment) would be good enough 👍

[asterite]

Sure, if someone wants to implement this, go ahead.

[paulcsmith]

@asterite I'd be happy to! Could you give me a pointer on where to hook into?

I imagine I'd hook in where the compiler decides whether to show "Did you mean?" for constants. There are a few places with the text "Did you mean", but I'm not sure which one is the one I should look at. Looks like it is here:

crystal/src/compiler/crystal/semantic/call_error.cr

Lines 23 to 37 in df279d1

	class Crystal::Path
	def raise_undefined_constant(type)
	private_const = type.lookup_path(self, include_private: true)
	if private_const
	self.raise("private constant #{private_const} referenced")
	end
	similar_name = type.lookup_similar_path(self)
	if similar_name
	self.raise("undefined constant #{self} #{type.program.colorize("(did you mean '#{similar_name}')").yellow.bold}")
	else
	self.raise("undefined constant #{self}")
	end
	end
	end

Is there a way to get at the path for the file where the compile error occurred? This would be useful for the more complex case if I decide to really stretch a bit :)

[asterite]

Path, like any other ast node, has a location property.

Location is a bit tricky because the file might refer to an expanded macro, but you can get the location where the macro was expanded.

But note that undefined constant might happen in other places too, I think.

[paulcsmith]

@asterite That's a great starting point. Thank you!

[sudo-nice]

I think, autoload could be a solution here:

1. One needs to mention a module/class name only once, no matter how many other files include that module/class and what the alphabetical order is.
2. It discourages monkey-patching.

The former issue never mentioned these two simple points.

[yxhuvud]

One aspect I haven't seen mentioned here is tooling. When trying to use tooling (for example macro expansion), you you have do a command line like crystal tool expand -c location/to/expand.cr:line:column location/to/expand.cr location/to/file/that/actually/define/or/require/definition. This is a bit hard to automate for editor integration currently - especially to get it working in both spec and regular compile targets.

This make me somewhat wonder if having a declartion driven solution would be neater - at least for the dependencies within a project. For dependencies to other shards require works fine and is quite free from tedium. There are some order dependencies here - open classes and whatnot but perhaps that could be solved, and I also don't know how to couple this with different requirement structures for different compile target sets (and having multiple targets is the norm as the spec suite is also a separate target). I see this as mostly a conflict about good ways to handle a project and good ways to handle single files being compiled.

Extra nice if it was possible to tell the compiler to expand a macro in a file from any compile target in the current project.

[jhass]

I'm in support of a more elaborated error message for missing constants.

Any magic beyond that I don't see as realistic. Having the compiler to pseudo require and parse all files it can find under some magic directory (right now src is only special to shards, not the compiler), is a no go. So we would need to either enforce a naming convention or go to an import based system, both of which I think are too big changes at the current stage of the language.