Deprecate eager-evaluated scopes.

Don't use this:

    scope :red, where(color: 'red')
    default_scope where(color: 'red')

Use this:

    scope :red, -> { where(color: 'red') }
    default_scope { where(color: 'red') }

The former has numerous issues. It is a common newbie gotcha to do
the following:

    scope :recent, where(published_at: Time.now - 2.weeks)

Or a more subtle variant:

    scope :recent, -> { where(published_at: Time.now - 2.weeks) }
    scope :recent_red, recent.where(color: 'red')

Eager scopes are also very complex to implement within Active
Record, and there are still bugs. For example, the following does
not do what you expect:

    scope :remove_conditions, except(:where)
    where(...).remove_conditions # => still has conditions

activerecord/CHANGELOG.md

@@ -1,5 +1,36 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Deprecate eager-evaluated scopes.
+
+    Don't use this:
+
+        scope :red, where(color: 'red')
+        default_scope where(color: 'red')
+
+    Use this:
+
+        scope :red, -> { where(color: 'red') }
+        default_scope { where(color: 'red') }
+
+    The former has numerous issues. It is a common newbie gotcha to do
+    the following:
+
+        scope :recent, where(published_at: Time.now - 2.weeks)
+
+    Or a more subtle variant:
+
+        scope :recent, -> { where(published_at: Time.now - 2.weeks) }
+        scope :recent_red, recent.where(color: 'red')
+
+    Eager scopes are also very complex to implement within Active
+    Record, and there are still bugs. For example, the following does
+    not do what you expect:
+
+        scope :remove_conditions, except(:where)
+        where(...).remove_conditions # => still has conditions
+
+    *Jon Leighton*
+
 *   Remove IdentityMap
 
     IdentityMap has never graduated to be an "enabled-by-default" feature, due

activerecord/lib/active_record/scoping/default.rb

@@ -1,4 +1,5 @@
 require 'active_support/concern'
+require 'active_support/deprecation'
 
 module ActiveRecord
   module Scoping
@@ -51,7 +52,7 @@ module ActiveRecord
         # the model.
         #
         #   class Article < ActiveRecord::Base
-        #     default_scope where(:published => true)
+        #     default_scope { where(:published => true) }
         #   end
         #
         #   Article.all # => SELECT * FROM articles WHERE published = true
@@ -62,12 +63,6 @@ module ActiveRecord
         #   Article.new.published    # => true
         #   Article.create.published # => true
         #
-        # You can also use <tt>default_scope</tt> with a block, in order to have it lazily evaluated:
-        #
-        #   class Article < ActiveRecord::Base
-        #     default_scope { where(:published_at => Time.now - 1.week) }
-        #   end
-        #
         # (You can also pass any object which responds to <tt>call</tt> to the <tt>default_scope</tt>
         # macro, and it will be called when building the default scope.)
         #
@@ -75,8 +70,8 @@ module ActiveRecord
         # be merged together:
         #
         #   class Article < ActiveRecord::Base
-        #     default_scope where(:published => true)
+        #     default_scope { where(:published => true) }
-        #     default_scope where(:rating => 'G')
+        #     default_scope { where(:rating => 'G') }
         #   end
         #
         #   Article.all # => SELECT * FROM articles WHERE published = true AND rating = 'G'
@@ -94,6 +89,16 @@ module ActiveRecord
         #   end
         def default_scope(scope = {})
           scope = Proc.new if block_given?
+
+          if scope.is_a?(Relation) || !scope.respond_to?(:call)
+            ActiveSupport::Deprecation.warn(
+              "Calling #default_scope without a block is deprecated. For example instead " \
+              "of `default_scope where(color: 'red')`, please use " \
+              "`default_scope { where(color: 'red') }`. (Alternatively you can just redefine " \
+              "self.default_scope.)"
+            )
+          end
+
           self.default_scopes = default_scopes + [scope]
         end
 
@@ -106,7 +111,7 @@ module ActiveRecord
                 if scope.is_a?(Hash)
                   default_scope.apply_finder_options(scope)
                 elsif !scope.is_a?(Relation) && scope.respond_to?(:call)
-                  default_scope.merge(scope.call)
+                  default_scope.merge(unscoped { scope.call })
                 else
                   default_scope.merge(scope)
                 end

activerecord/lib/active_record/scoping/named.rb

@@ -3,6 +3,7 @@ require 'active_support/core_ext/hash/except'
 require 'active_support/core_ext/kernel/singleton_class'
 require 'active_support/core_ext/object/blank'
 require 'active_support/core_ext/class/attribute'
+require 'active_support/deprecation'
 
 module ActiveRecord
   # = Active Record Named \Scopes
@@ -171,11 +172,24 @@ module ActiveRecord
         #   Article.published.featured.latest_article
         #   Article.featured.titles
 
-        def scope(name, scope_options = {}, &block)
+        def scope(name, body = {}, &block)
           extension = Module.new(&block) if block
 
+          # Check body.is_a?(Relation) to prevent the relation actually being
+          # loaded by respond_to?
+          if body.is_a?(Relation) || !body.respond_to?(:call)
+            ActiveSupport::Deprecation.warn(
+              "Using #scope without passing a callable object is deprecated. For " \
+              "example `scope :red, where(color: 'red')` should be changed to " \
+              "`scope :red, -> { where(color: 'red') }`. There are numerous gotchas " \
+              "in the former usage and it makes the implementation more complicated " \
+              "and buggy. (If you prefer, you can just define a class method named " \
+              "`self.red`.)"
+            )
+          end
+
           singleton_class.send(:define_method, name) do |*args|
-            options = scope_options.respond_to?(:call) ? unscoped { scope_options.call(*args) } : scope_options
+            options = body.respond_to?(:call) ? unscoped { body.call(*args) } : body
             options = scoped.apply_finder_options(options) if options.is_a?(Hash)
 
             relation = scoped.merge(options)

activerecord/test/cases/named_scope_test.rb

@@ -123,16 +123,6 @@ class NamedScopeTest < ActiveRecord::TestCase
     assert objects.all?(&:approved?), 'all objects should be approved'
   end
 
-  def test_extensions
-    assert_equal 1, Topic.anonymous_extension.one
-    assert_equal 2, Topic.named_extension.two
-  end
-
-  def test_multiple_extensions
-    assert_equal 2, Topic.multiple_extensions.extension_two
-    assert_equal 1, Topic.multiple_extensions.extension_one
-  end
-
   def test_has_many_associations_have_access_to_scopes
     assert_not_equal Post.containing_the_letter_a, authors(:david).posts
     assert !Post.containing_the_letter_a.empty?
@@ -464,6 +454,41 @@ class NamedScopeTest < ActiveRecord::TestCase
       require "models/without_table"
     end
   end
+
+  def test_eager_scopes_are_deprecated
+    klass = Class.new(ActiveRecord::Base)
+    klass.table_name = 'posts'
+
+    assert_deprecated do
+      klass.scope :welcome, { :conditions => { :id => posts(:welcome).id } }
+    end
+    assert_equal [posts(:welcome).title], klass.welcome.map(&:title)
+
+    assert_deprecated do
+      klass.scope :welcome_2, klass.where(:id => posts(:welcome).id)
+    end
+    assert_equal [posts(:welcome).title], klass.welcome_2.map(&:title)
+  end
+
+  def test_eager_default_scope_hashes_are_deprecated
+    klass = Class.new(ActiveRecord::Base)
+    klass.table_name = 'posts'
+
+    assert_deprecated do
+      klass.send(:default_scope, :conditions => { :id => posts(:welcome).id })
+    end
+    assert_equal [posts(:welcome).title], klass.all.map(&:title)
+  end
+
+  def test_eager_default_scope_relations_are_deprecated
+    klass = Class.new(ActiveRecord::Base)
+    klass.table_name = 'posts'
+
+    assert_deprecated do
+      klass.send(:default_scope, klass.where(:id => posts(:welcome).id))
+    end
+    assert_equal [posts(:welcome).title], klass.all.map(&:title)
+  end
 end
 
 class DynamicScopeMatchTest < ActiveRecord::TestCase

activerecord/test/models/author.rb

@@ -140,8 +140,8 @@ class Author < ActiveRecord::Base
   has_many :posts_with_default_include, :class_name => 'PostWithDefaultInclude'
   has_many :comments_on_posts_with_default_include, :through => :posts_with_default_include, :source => :comments
 
-  scope :relation_include_posts, includes(:posts)
+  scope :relation_include_posts, -> { includes(:posts) }
-  scope :relation_include_tags, includes(:tags)
+  scope :relation_include_tags,  -> { includes(:tags) }
 
   attr_accessor :post_log
   after_initialize :set_post_log

activerecord/test/models/bulb.rb

@@ -1,5 +1,5 @@
 class Bulb < ActiveRecord::Base
-  default_scope where(:name => 'defaulty')
+  default_scope { where(:name => 'defaulty') }
   belongs_to :car
 
   attr_protected :car_id, :frickinawesome

activerecord/test/models/car.rb

@@ -11,18 +11,18 @@ class Car < ActiveRecord::Base
   has_many :engines, :dependent => :destroy
   has_many :wheels, :as => :wheelable, :dependent => :destroy
 
-  scope :incl_tyres, includes(:tyres)
+  scope :incl_tyres, -> { includes(:tyres) }
-  scope :incl_engines, includes(:engines)
+  scope :incl_engines, -> { includes(:engines) }
 
-  scope :order_using_new_style,  order('name asc')
+  scope :order_using_new_style,  -> { order('name asc') }
-  scope :order_using_old_style,  :order => 'name asc'
+  scope :order_using_old_style,  -> { { :order => 'name asc' } }
 
 end
 
 class CoolCar < Car
-  default_scope :order => 'name desc'
+  default_scope { order('name desc') }
 end
 
 class FastCar < Car
-  default_scope :order => 'name desc'
+  default_scope { order('name desc') }
 end

activerecord/test/models/categorization.rb

@@ -12,7 +12,7 @@ end
 
 class SpecialCategorization < ActiveRecord::Base
   self.table_name = 'categorizations'
-  default_scope where(:special => true)
+  default_scope { where(:special => true) }
 
   belongs_to :author
   belongs_to :category

activerecord/test/models/category.rb

@@ -27,7 +27,7 @@ class Category < ActiveRecord::Base
   has_many :authors, :through => :categorizations
   has_many :authors_with_select, :through => :categorizations, :source => :author, :select => 'authors.*, categorizations.post_id'
 
-  scope :general, :conditions => { :name => 'General' }
+  scope :general, -> { where(:name => 'General') }
 end
 
 class SpecialCategory < Category

activerecord/test/models/comment.rb

@@ -1,12 +1,10 @@
 class Comment < ActiveRecord::Base
   scope :limit_by, lambda {|l| limit(l) }
-  scope :containing_the_letter_e, :conditions => "comments.body LIKE '%e%'"
+  scope :containing_the_letter_e, -> { where("comments.body LIKE '%e%'") }
-  scope :not_again, where("comments.body NOT LIKE '%again%'")
+  scope :not_again, -> { where("comments.body NOT LIKE '%again%'") }
-  scope :for_first_post, :conditions => { :post_id => 1 }
+  scope :for_first_post, -> { where(:post_id => 1) }
-  scope :for_first_author,
+  scope :for_first_author, -> { joins(:post).where("posts.author_id" => 1) }
-              :joins => :post,
+  scope :created, -> { scoped }
-              :conditions => { "posts.author_id" => 1 }
-  scope :created
 
   belongs_to :post, :counter_cache => true
   has_many :ratings
@@ -25,7 +23,7 @@ class Comment < ActiveRecord::Base
   def self.all_as_method
     all
   end
-  scope :all_as_scope, {}
+  scope :all_as_scope, -> { scoped }
 end
 
 class SpecialComment < Comment

activerecord/test/models/developer.rb

@@ -45,7 +45,7 @@ class Developer < ActiveRecord::Base
 
   has_many :audit_logs
 
-  scope :jamises, :conditions => {:name => 'Jamis'}
+  scope :jamises, -> { where(:name => 'Jamis') }
 
   validates_inclusion_of :salary, :in => 50000..200000
   validates_length_of    :name, :within => 3..20
@@ -88,20 +88,20 @@ end
 
 class DeveloperWithSelect < ActiveRecord::Base
   self.table_name = 'developers'
-  default_scope select('name')
+  default_scope { select('name') }
 end
 
 class DeveloperWithIncludes < ActiveRecord::Base
   self.table_name = 'developers'
   has_many :audit_logs, :foreign_key => :developer_id
-  default_scope includes(:audit_logs)
+  default_scope { includes(:audit_logs) }
 end
 
 class DeveloperOrderedBySalary < ActiveRecord::Base
   self.table_name = 'developers'
-  default_scope :order => 'salary DESC'
+  default_scope { order('salary DESC') }
 
-  scope :by_name, order('name DESC')
+  scope :by_name, -> { order('name DESC') }
 
   def self.all_ordered_by_name
     with_scope(:find => { :order => 'name DESC' }) do
@@ -112,7 +112,7 @@ end
 
 class DeveloperCalledDavid < ActiveRecord::Base
   self.table_name = 'developers'
-  default_scope where("name = 'David'")
+  default_scope { where("name = 'David'") }
 end
 
 class LazyLambdaDeveloperCalledDavid < ActiveRecord::Base
@@ -140,7 +140,7 @@ end
 
 class ClassMethodReferencingScopeDeveloperCalledDavid < ActiveRecord::Base
   self.table_name = 'developers'
-  scope :david, where(:name => 'David')
+  scope :david, -> { where(:name => 'David') }
 
   def self.default_scope
     david
@@ -149,40 +149,40 @@ end
 
 class LazyBlockReferencingScopeDeveloperCalledDavid < ActiveRecord::Base
   self.table_name = 'developers'
-  scope :david, where(:name => 'David')
+  scope :david, -> { where(:name => 'David') }
   default_scope { david }
 end
 
 class DeveloperCalledJamis < ActiveRecord::Base
   self.table_name = 'developers'
 
-  default_scope where(:name => 'Jamis')
+  default_scope { where(:name => 'Jamis') }
-  scope :poor, where('salary < 150000')
+  scope :poor, -> { where('salary < 150000') }
 end
 
 class PoorDeveloperCalledJamis < ActiveRecord::Base
   self.table_name = 'developers'
 
-  default_scope where(:name => 'Jamis', :salary => 50000)
+  default_scope -> { where(:name => 'Jamis', :salary => 50000) }
 end
 
 class InheritedPoorDeveloperCalledJamis < DeveloperCalledJamis
   self.table_name = 'developers'
 
-  default_scope where(:salary => 50000)
+  default_scope -> { where(:salary => 50000) }
 end
 
 class MultiplePoorDeveloperCalledJamis < ActiveRecord::Base
   self.table_name = 'developers'
 
-  default_scope where(:name => 'Jamis')
+  default_scope -> { where(:name => 'Jamis') }
-  default_scope where(:salary => 50000)
+  default_scope -> { where(:salary => 50000) }
 end
 
 module SalaryDefaultScope
   extend ActiveSupport::Concern
 
-  included { default_scope where(:salary => 50000) }
+  included { default_scope { where(:salary => 50000) } }
 end
 
 class ModuleIncludedPoorDeveloperCalledJamis < DeveloperCalledJamis
@@ -195,7 +195,7 @@ class EagerDeveloperWithDefaultScope < ActiveRecord::Base
   self.table_name = 'developers'
   has_and_belongs_to_many :projects, :foreign_key => 'developer_id', :join_table => 'developers_projects', :order => 'projects.id'
 
-  default_scope includes(:projects)
+  default_scope { includes(:projects) }
 end
 
 class EagerDeveloperWithClassMethodDefaultScope < ActiveRecord::Base

activerecord/test/models/organization.rb

@@ -8,5 +8,5 @@ class Organization < ActiveRecord::Base
   has_one :author, :primary_key => :name
   has_one :author_owned_essay_category, :through => :author, :source => :owned_essay_category
 
-  scope :clubs, { :from => 'clubs' }
+  scope :clubs, -> { from('clubs') }
 end

activerecord/test/models/person.rb

@@ -27,8 +27,8 @@ class Person < ActiveRecord::Base
   has_many :agents_posts,         :through => :agents,       :source => :posts
   has_many :agents_posts_authors, :through => :agents_posts, :source => :author
 
-  scope :males,   :conditions => { :gender => 'M' }
+  scope :males,   -> { where(:gender => 'M') }
-  scope :females, :conditions => { :gender => 'F' }
+  scope :females, -> { where(:gender => 'F') }
 end
 
 class PersonWithDependentDestroyJobs < ActiveRecord::Base

activerecord/test/models/post.rb

@@ -5,8 +5,8 @@ class Post < ActiveRecord::Base
     end
   end
 
-  scope :containing_the_letter_a, where("body LIKE '%a%'")
+  scope :containing_the_letter_a, -> { where("body LIKE '%a%'") }
-  scope :ranked_by_comments, order("comments_count DESC")
+  scope :ranked_by_comments,      -> { order("comments_count DESC") }
 
   scope :limit_by, lambda {|l| limit(l) }
   scope :with_authors_at_address, lambda { |address| {
@@ -30,8 +30,8 @@ class Post < ActiveRecord::Base
   has_one :first_comment, :class_name => 'Comment', :order => 'id ASC'
   has_one :last_comment, :class_name => 'Comment', :order => 'id desc'
 
-  scope :with_special_comments, :joins => :comments, :conditions => {:comments => {:type => 'SpecialComment'} }
+  scope :with_special_comments, -> { joins(:comments).where(:comments => {:type => 'SpecialComment'}) }
-  scope :with_very_special_comments, joins(:comments).where(:comments => {:type => 'VerySpecialComment'})
+  scope :with_very_special_comments, -> { joins(:comments).where(:comments => {:type => 'VerySpecialComment'}) }
   scope :with_post, lambda {|post_id|
     { :joins => :comments, :conditions => {:comments => {:post_id => post_id} } }
   }
@@ -171,7 +171,7 @@ end
 
 class FirstPost < ActiveRecord::Base
   self.table_name = 'posts'
-  default_scope where(:id => 1)
+  default_scope { where(:id => 1) }
 
   has_many :comments, :foreign_key => :post_id
   has_one  :comment,  :foreign_key => :post_id
@@ -179,16 +179,16 @@ end
 
 class PostWithDefaultInclude < ActiveRecord::Base
   self.table_name = 'posts'
-  default_scope includes(:comments)
+  default_scope { includes(:comments) }
   has_many :comments, :foreign_key => :post_id
 end
 
 class PostWithDefaultScope < ActiveRecord::Base
   self.table_name = 'posts'
-  default_scope :order => :title
+  default_scope { order(:title) }
 end
 
 class SpecialPostWithDefaultScope < ActiveRecord::Base
   self.table_name = 'posts'
-  default_scope where(:id => [1, 5,6])
+  default_scope { where(:id => [1, 5,6]) }
 end

activerecord/test/models/project.rb

@@ -32,7 +32,7 @@ class Project < ActiveRecord::Base
   def self.all_as_method
     all
   end
-  scope :all_as_scope, {}
+  scope :all_as_scope, -> { scoped }
 end
 
 class SpecialProject < Project

activerecord/test/models/reference.rb

@@ -19,5 +19,5 @@ end
 
 class BadReference < ActiveRecord::Base
   self.table_name = 'references'
-  default_scope where(:favourite => false)
+  default_scope { where(:favourite => false) }
 end

activerecord/test/models/reply.rb

@@ -1,7 +1,7 @@
 require 'models/topic'
 
 class Reply < Topic
-  scope :base
+  scope :base, -> { scoped }
 
   belongs_to :topic, :foreign_key => "parent_id", :counter_cache => true
   belongs_to :topic_with_primary_key, :class_name => "Topic", :primary_key => "title", :foreign_key => "parent_title", :counter_cache => "replies_count"

activerecord/test/models/topic.rb

@@ -1,21 +1,24 @@
 class Topic < ActiveRecord::Base
-  scope :base
+  scope :base, -> { scoped }
   scope :written_before, lambda { |time|
     if time
       { :conditions => ['written_on < ?', time] }
     end
   }
-  scope :approved, :conditions => {:approved => true}
+  scope :approved, -> { where(:approved => true) }
-  scope :rejected, :conditions => {:approved => false}
+  scope :rejected, -> { where(:approved => false) }
 
   scope :scope_with_lambda, lambda { scoped }
 
-  scope :by_lifo, :conditions => {:author_name => 'lifo'}
+  scope :by_lifo, -> { where(:author_name => 'lifo') }
 
-  scope :approved_as_hash_condition, :conditions => {:topics => {:approved => true}}
+  ActiveSupport::Deprecation.silence do
-  scope 'approved_as_string', :conditions => {:approved => true}
+    scope :approved_as_hash_condition, :conditions => {:topics => {:approved => true}}
-  scope :replied, :conditions => ['replies_count > 0']
+    scope :replied, :conditions => ['replies_count > 0']
-  scope :anonymous_extension do
+  end
+
+  scope 'approved_as_string', -> { where(:approved => true) }
+  scope :anonymous_extension, -> { scoped } do
     def one
       1
     end
@@ -42,8 +45,8 @@ class Topic < ActiveRecord::Base
       2
     end
   end
-  scope :named_extension, :extend => NamedExtension
+  scope :named_extension, -> { { :extend => NamedExtension } }
-  scope :multiple_extensions, :extend => [MultipleExtensionTwo, MultipleExtensionOne]
+  scope :multiple_extensions, -> { { :extend => [MultipleExtensionTwo, MultipleExtensionOne] } }
 
   has_many :replies, :dependent => :destroy, :foreign_key => "parent_id"
   has_many :replies_with_primary_key, :class_name => "Reply", :dependent => :destroy, :primary_key => "title", :foreign_key => "parent_title"

activerecord/test/models/toy.rb

@@ -2,5 +2,5 @@ class Toy < ActiveRecord::Base
   self.primary_key = :toy_id
   belongs_to :pet
 
-  scope :with_pet, joins(:pet)
+  scope :with_pet, -> { joins(:pet) }
 end

activerecord/test/models/without_table.rb

@@ -1,3 +1,3 @@
 class WithoutTable < ActiveRecord::Base
-  default_scope where(:published => true)
+  default_scope -> { where(:published => true) }
 end