Implement the new RelativeRequireToLib cop + add tests

[utkarsh2102]

TODO:

• document stuff.
• fix regex to match proper calls. use File.expand_path to check if the expanded path lies inside the lib directory or not.
• find a way to only check calls in the spec(s)/test(s) dir.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>

[deivid-rodriguez]

Rather than reviewing the specifics, I'll do a recap of the expectations to make sure we're all on the same page.

I see two "families of cases" we are interested in. One of them "solvable" and one of the "ambiguous".

When the require call does not depend on the CWD

This case is the one we can reliably solve without false positive. Examples:

• require with an absolute path from a file inside specs that falls into lib. For example, require "/path/to/file/inside/lib/foo.rb". This case happens hardly ever in real life but we should be able to reduce more complex cases to this one.

• require with a File.expand_path call based on __dir__ or __FILE__. For example, require File.expand_path("../lib/foo.rb", __dir__).

• Any call to require_relative (which implicitly expands its argument from __dir__).

I believe to solve this case in general we need to be able to access the path to current file being inspected, since that will give us the real value of __dir__ and __FILE__.

I had a quick look at some rubocop cops that use this information, and we should be able to get this with:

def investigate(processed_source)
  we_need_this = processed_source.file_path
  # ...
end

When the require call depends on the CWD

Unfortunately, this case cannot be solved reliably, because the CWD is only known at runtime. For example, require "../lib/foo.rb" is an offense if the code is run from specs, but it's not an offense if the code is run from specs/foo.

These cases are pretty uncommon and involve any calls to require where the argument starts with a dot (.).

My proposal would be to give up on this for now, and if we have time, implement a more general cop that flags any calls to require that depend on the CWD. It should be a very easy cop anyways, just check if require is used, and the argument starts with ..

[terceiro]

thanks @deivid-rodriguez for the thoughts you put into this. for the "When the require call depends on the CWD" case, I think we should also emit an offense because people should not be doing this. it can even be exploited for security flaws.

[deivid-rodriguez]

Yeah, I think it's a good cop idea. But I believe this could be added upstream since it's not specific to packaging.

[deivid-rodriguez]

In any case, that case is super simple, just check that the require argument starts with a dot. The could be other cases, like require Pathname(<foo>).relative_path_from(<bar>), but that would pick most offenses.

[terceiro]

LGTM

[marcandre]

FYI, I wrote this ruby-style-guide entry which I think goes exactly against this cop... Let me know if the justification isn't clear enough.

[deivid-rodriguez]

Hi @marcandre!

Thanks for your help here and for writing that guideline in the style guide. We're aware of that best practice and we do support that. As a matter of fact I've been migrating code over the years to follow that 👍.

With this cop, we don't intend to break that rule in general, only to add one exception to it. It turns out that for gem packagers it's useful to be able to "break" a gem into its different components (lib/, test/, exe/, and so on), in order to fit the different layouts in which gems can be installed as OS packages. So, when relative_require is used "across components", making assumptions on how the components will be installed relatively to each other, this sometimes forces gem packagers to monkeypatch gems to not do that, and instead rely on the $LOAD_PATH setup.

The cases we are preventing don't look great to me as relative_require's anyways, because you have to go several levels down, and then up again, which is not as straightforward as requiring stuff relatively inside the same subtree. And the performance gain doesn't seem like a big deal to me either. It's only test performance, not runtime performance, and I don't think there's usually enough of these requires so that this makes any noticiable difference in your tests.

A similar situation, but that affects not only packaging contexts, would be requiring your lib code from your gem executable relatively. That wouldn't work for default gems, because the layout of the installed gem doesn't match the development layout of the gem.

Anwyays, what I mean is that we don't intend to go against that rule in general, only to provide a tool that people interested in getting their gems more easily packaged for OSs can follow.

[deivid-rodriguez]

Nice work!

[deivid-rodriguez]

Also, since it might not be clear from my previous message, and from the cops implementation and naming. This cop only intends to detect requires from the specs folder to the lib folder. So, it should only ever be configured with

Include:
  - 'specs/**/*.rb'

or

Include:
  - 'test/**/*.rb'

Maybe we could validate the configuration of the cop to make sure it doesn't include any files inside lib, and rename the cop to something like RequireRelativeOfLibFileFromOutsideOfLibFolder, or something like that, to make this point more clear.

[utkarsh2102]

Thanks for the feedback so far! 😄
It looks like all these details could be provided via "good documentation" and perhaps that's what I am going to focus on after this.

I am going to merge this and carry on with the next tasks at hand, starting with documenting things properly! 🚀
(we can always keep discussing more about the guide and other things here..)