LetDefinitions needs to be defined on the nested instance rather than the definition of ExampleGroup

[JonRowe]

In v2.13.0 let defintion with rspec-rails is broken when shadowing definitions in helper modules.

In rspec-core the blocks passed to let generate methods on a LetDefinitions module (see here), which is delegated to by the memoization. With rspec-rails these definitions are occurring on a top level ExampleGroup::LetDefintions module rather than being defined on the anonymous nested instances. This means that helper methods which shadow the let name will be memozied instead of the let's block.

The symptom of this originally popped up in rspec-core (rspec/rspec-core#817) but I was unable to replicate it without rspec-rails so I believe the issue lies here. Alternatively we need to change how we define let's again.

I've created this branch on rspec-rails rather than my fork because I need feedback on how this could be occurring, I'm not familiar enough with the internals. The feature attached is not expected to be included in any final patch, it's just for demonstation of the problem.

So... Thoughts? @alindeman @samphippen ...

[myronmarston]

I suspect this is the source of #737 as well.

[alindeman]

:( Help wanted here, though I'll try to dig in soon.

[myronmarston]

I think I just realized what the problem is. It's actually not rspec-rails specific, but it is exacerbated by all the modules rspec rails mixes in to provide helper methods.

In 2.13, we changed how let works to support super. The technique used was to define the method in a module that is mixed in, then define an override that does the memoization and accesses the user's logic using super. (This is necessary so that the user's block is passed directly to define_method -- that's the only way to support method constructs like super).

As a result, if a let is defined that has the same name as another helper method, the memoization definition will super to the existing helper method and not the user's definition.

The question is, what should we do about it?

I think it would be very complicated to make the user's let take precedence. Instead, I would advocate having rspec-core issue a warning and/or an error if the user is defining a let with a name that has an existing method. Really, if you think about it, there are other issues with doing so, even if it worked...for example, rack-test makes a last_response helper available. If a user naively defined a let(:last_response) and it overrode rack-test's, then there may be other methods provided by rack-test that depend upon last_response that would mysteriously stop working. (Actually, consider that this is the basic issue reported in rspec/rspec-mocks#102). One slight complication: while I think we don't want to allow a let to override a helper method, we do want it to be able to override a let definition from a parent group (that was the point of adding support for super, after all, that triggered this whole thing...).

Thoughts?

[JonRowe]

@myronmarston As I said over on rspec/rspec-core#817 the problem is the ordering of the example group ancestors particularly the let definition module vs the helper modules. On pure rspec-core the module sits in the example group cluster at the top of ancestors, so has precedence already, only on rspec-rails does it get defined at the bottom.

I believe that the problem you outlined with let(:last_response) is already present, but would only affect users, as it wouldn't affect outside code, because the let's definition is only defined in the top few layers of example group.

[myronmarston]

On pure rspec-core the module sits in the example group cluster at the top of ancestors, so has precedence already, only on rspec-rails does it get defined at the bottom.

Can you point me to the code that changes the order of the ancestors? I don't understand this.

[JonRowe]

@myronmarston That's what I need help with, I can't seem to find any that would do it. See https://gist.github.com/JonRowe/5368859 for the changes

[alindeman]

OK, I think I see what's going on here, but I'm going to leave some raw analysis for @JonRowe and @myronmarston ... and revisit it myself in the morning as my brain is shot for the night :)

Many Rails example groups define included hooks that invoke subject { ... }. For example, ControllerExampleGroup makes the controller its subject by default. It is desirable to define subject at a high level like this so it's a default, but can be overridden by users.

That said, calling subject also causes this problem because subject is implemented in terms of let, and the first time let is called is when LetDefinitions is defined and put into the ancestor chain.

Because subject is called so soon in many contexts in rspec-rails, LetDefinitions ends up being included before modules included via RSpec.configuration.include, meaning LetDefinitions has less precedence than those modules included later.

[alindeman]

I'm going to try an experiment in rspec-core where I try to defer the inclusion of the module that defines let definitions. I'm still open to additional thoughts or analysis, @myronmarston and @JonRowe.

Again, thanks so much for this test case. It's helped me very quickly understand what was happening.

[myronmarston]

@alindeman -- that sounds worthwhile. I commented above about this, but do you think it would also be worthwhile to raise an error and/or print a warning if let is used to overwrite a non-let method? In the case of something like last_response, which rack-test provides, and expects to behave in a certain fashion, it can cause problems if a user unknowingly overwrites it with let(:last_response).

[alindeman]

@myronmarston, I have a feeling that many folks may depend on the ability to override methods via let. I don't think it should be an error, and I'm even a tad uncomfortable with a warning. But I might be convinced with some more thoughts.

I have pushed a fix that uses a method named subject rather than using the subject { ... } method from MemoizedHelpers. This change defers the creation and inclusion of LetDefinitions until after other modules have been brought in. it's a decent short-term fix, I think.

Notably, I believe it is still possible to cause weird behavior without rspec-rails by defining your own modules that have self.included hooks that call let or subject. Whenever the first let or subject is called is when LetDefinitions is created and put in the ancestor chain. I tried a few ways to defer inclusion of LetDefinitions in rspec-core, but came up short: my solutions were either very complicated, or didn't quite work (or both). I'm happy to pair with someone on this for a longer-term solution.

[alindeman]

I've merged this for now, but feel free to keep the discussion going here or elsewhere.

[JonRowe]

I feel that's a definite lul wat... moment, I think we might need to revisit how this works to clean up once and for all...

[alindeman]

It's on my TODO to make a test case and file a bug against rspec-core.

[myronmarston]

FWIW, I've got some thoughts on how to fix this in core.