1190

オブジェクトコンストラクタから仮想メンバへの呼び出しについて、ReSharperから警告を受けています。

なぜこれはしてはいけないのでしょうか。


  • @ m.edmondson、真剣に..あなたのコメントがここに答えになるはずです。グレッグの説明は正しいが、私はあなたのブログを読むまでそれを理解しなかった。 - Rosdi Kasim
  • ドメインの有効期限が切れています。 - Robert Noack
  • @ m.edmondsonからの記事がここにあります。codeproject.com/Articles/802375/… - SpeziFish

17 답변


1072

C#で書かれたオブジェクトが構築されると、初期化子は最も派生クラスから基本クラスへと順番に実行され、次にコンストラクタは基本クラスから最も派生クラスへと実行されるこれがなぜなのかについての詳細はEric Lippertのブログを見てください。

また.NETでは、オブジェクトは構築時に型を変更するのではなく、最も派生型として開始し、メソッド表は最も派生型になります。つまり、仮想メソッド呼び出しは常に最も派生型で実行されます。

これら2つの事実を組み合わせると、コンストラクタ内で仮想メソッド呼び出しを行い、それが継承階層で最も派生型ではない場合、コンストラクタが設定されていないクラスで呼び出されるという問題が残ります。そのため、そのメソッドが呼び出されるのに適した状態になっていない可能性があります。

継承階層で最も派生型であることを保証するためにクラスをシール済みとしてマークした場合、この問題はもちろん軽減されます。その場合、仮想メソッドを呼び出しても安全です。


  • グレッグ、VIRTUALメンバーがある場合、なぜ誰かがSEALEDクラス(INHERITEDにすることはできません)を持つことになるのですか?[DERIVEDクラスでオーバーライドする]。 - Paul Pacurar
  • 派生クラスをそれ以上派生させることができないようにするには、それを封印しても問題ありません。 - Øyvind
  • @Paul - 重要なのは、の仮想メンバーを派生し終えたということです。ベースclass [es]は、クラスが完全に派生したものとしてマークしています。 - ljs
  • @Greg仮想メソッドの動作がインスタンス変数と関係がない場合は、問題ありませんか。仮想メソッドがインスタンス変数を変更しないことを宣言できたらいいのでしょうか。 (static?)たとえば、より派生型をインスタンス化するためにオーバーライドできる仮想メソッドを使用したい場合などです。これは私にとっては安全なようであり、この警告を保証するものではありません。 - Dave Cousineau
  • @PaulPacurar - 最も派生クラスの仮想メソッドを呼び出したい場合でも、問題が発生しないことがわかっている場合でも警告が表示されます。その場合、あなたはそのクラスを封印することによってあなたの知識をシステムと共有することができます。 - Revolutionair

527

あなたの質問に答えるために、この質問を考慮してください。Childオブジェクトはインスタンス化されている?

class Parent
{
    public Parent()
    {
        DoSomething();
    }

    protected virtual void DoSomething() 
    {
    }
}

class Child : Parent
{
    private string foo;

    public Child() 
    { 
        foo = "HELLO"; 
    }

    protected override void DoSomething()
    {
        Console.WriteLine(foo.ToLower()); //NullReferenceException!?!
    }
}

答えは、実際にはNullReferenceExceptionスローされるのは、foo無効です。オブジェクトの基本コンストラクタは、それ自身のコンストラクタの前に呼び出されます。。を持つことによってvirtualオブジェクトのコンストラクタを呼び出すと、継承しているオブジェクトが完全に初期化される前にコードを実行する可能性があります。


  • マークされたものより良い答え。 - Hele
  • これは上記の答えよりも明らかです。サンプルコードは1000語の価値があります。 - Novice in.NET
  • そうですね、いい例ですね。 - Joris Brauns

156

C#の規則は、JavaやC ++の規則とは大きく異なります。

C#のオブジェクトのコンストラクタにいるとき、そのオブジェクトは完全に派生した型として、完全に初期化された(単に「構築された」ものではない)形式で存在します。

namespace Demo
{
    class A 
    {
      public A()
      {
        System.Console.WriteLine("This is a {0},", this.GetType());
      }
    }

    class B : A
    {      
    }

    // . . .

    B b = new B(); // Output: "This is a Demo.B"
}

つまり、Aのコンストラクターから仮想関数を呼び出すと、Bのオーバーライドが提供されていれば、そのオーバーライドが解決されます。

システムの動作を完全に理解して、意図的にAとBをこのように設定したとしても、後でショックを受ける可能性があります。 Bのコンストラクタで仮想関数を呼び出し、それらがBまたはAによって適切に処理されることを「知って」いるとします。それから時間が経ち、他の誰かがCを定義し、そこで仮想関数のいくつかをオーバーライドする必要があると決心します。突然のBのコンストラクタはすべてCでコードを呼び出すことになり、これは非常に驚くべき動作につながる可能性があります。

とにかくコンストラクタの中で仮想関数を避けることはおそらく良い考えです。ありますC#、C ++、そしてJavaの間ではとても違います。あなたのプログラマは何を期待すべきかわからないかもしれません!


  • Greg Beechの回答、残念ながら私の回答ほど多くは投票されていませんが、私はより良い回答だと感じています。それには時間がかからなかったので、確かにもう少し有益で説明的な詳細がいくつかあります。 - Lloyd
  • 実際には、Javaの規則は同じです。 - OlegYch
  • @ Joã oPortela C ++は実際には非常に異なります。コンストラクタ(およびデストラクタ!)での仮想メソッド呼び出しは、現在構築中の型(およびvtable)を使用して解決されます。JavaおよびC#のように最も派生型ではありません。これが関連FAQエントリです。。 - Jacek Sieka
  • @ JacekSiekaあなたは絶対に正しいです。 C ++でコーディングして以来、しばらく時間がかかりました。他の人を混乱させないためにコメントを削除する必要がありますか? - João Portela
  • C#がJavaとVB.NETの両方と異なる点は大きく異なります。 C#では、宣言の時点で初期化されるフィールドは、基本コンストラクター呼び出しの前に初期化が処理されます。これは、派生クラスオブジェクトをコンストラクタから使用できるようにする目的で行われましたが、残念ながら、そのような機能は、初期化が派生クラスパラメータによって制御されない派生クラス機能に対してのみ機能します。 - supercat

83

警告の理由はすでに説明されていますが、警告をどのように修正しますか。あなたはクラスか仮想メンバーのどちらかを封印しなければなりません。

  class B
  {
    protected virtual void Foo() { }
  }

  class A : B
  {
    public A()
    {
      Foo(); // warning here
    }
  }

あなたはクラスAを封印することができます:

  sealed class A : B
  {
    public A()
    {
      Foo(); // no warning
    }
  }

あるいは、メソッドFooを封印することもできます。

  class A : B
  {
    public A()
    {
      Foo(); // no warning
    }

    protected sealed override void Foo()
    {
      base.Foo();
    }
  }


16

C#では、基本クラスのコンストラクタが実行されます。派生クラスのコンストラクタ。派生クラスがオーバーライドされる可能性のある仮想メンバーで使用する可能性のあるインスタンスフィールドはまだ初期化されていません。

これはただの警告あなたが注意を払うようにし、それが大丈夫であることを確認するために。このシナリオには実際のユースケースがあります。動作を文書化するそれが呼び出しているコンストラクタがあるところの下の派生クラスで宣言されたインスタンスフィールドを使用することはできないという仮想メンバの。


11

なぜあなたのために上記のよく書かれた答えがありますしないそれをやりたい。これはおそらくあなたがいる反例です。だろうやりたいこと(C#からに翻訳)Rubyにおける実用的なオブジェクト指向設計著Sandi Metz、p。 126)。

ご了承くださいGetDependency()インスタンス変数には触れません。静的メソッドが仮想の場合、静的になります。

(公平を期すために、依存性注入コンテナまたはオブジェクト初期化子を介してこれを行うより賢い方法がおそらくあります...)

public class MyClass
{
    private IDependency _myDependency;

    public MyClass(IDependency someValue = null)
    {
        _myDependency = someValue ?? GetDependency();
    }

    // If this were static, it could not be overridden
    // as static methods cannot be virtual in C#.
    protected virtual IDependency GetDependency() 
    {
        return new SomeDependency();
    }
}

public class MySubClass : MyClass
{
    protected override IDependency GetDependency()
    {
        return new SomeOtherDependency();
    }
}

public interface IDependency  { }
public class SomeDependency : IDependency { }
public class SomeOtherDependency : IDependency { }


  • 私はこれのためにファクトリメソッドを使うことを検討しているでしょう。 - Ian Ringrose
  • ほとんど役に立たないものを含めるのではなく、.NET Frameworkにしてほしいのですが。FinalizeのデフォルトメンバーとしてObject、そのvtableスロットを使用していたManageLifetime(LifetimeStatus)コンストラクタがクライアントコードに戻るとき、コンストラクタがスローされるとき、またはオブジェクトが放棄されたことが判明したときに呼び出されるメソッド。基本クラスのコンストラクターから仮想メソッドを呼び出すことを伴うほとんどのシナリオは、2段階構成を使用して最もうまく処理できますが、2段階構成は、クライアントが2段階を呼び出すという要件ではなく、実装の詳細として動作するはずです。 - supercat
  • それでも、このスレッドで示されている他のケースと同じように、このコードで問題が発生する可能性があります。GetDependency以前に呼び出しても安全であるとは保証されていません。MySubClassコンストラクターが呼び出されました。また、デフォルトの依存関係をデフォルトでインスタンス化することは、「純粋なDI」と呼ぶものではありません。 - Groo
  • この例では、「依存関係の排除」を行います。 ;-)私にとってこれはコンストラクタからの仮想メソッド呼び出しのもう一つの良い反例です。 SomeDependencyは、MySubClass派生でインスタンス化されなくなり、SomeDependencyに依存するすべてのMyClass機能の動作が壊れます。 - Nachbars Lumpi

5

はい、コンストラクタで仮想メソッドを呼び出すのは一般的には良くありません。

この時点で、オブジェクトはまだ完全に構築されていない可能性があり、メソッドによって期待される不変式はまだ成り立たない可能性があります。


5

あなたのコンストラクタは(後で、あなたのソフトウェアの拡張で)仮想メソッドをオーバーライドするサブクラスのコンストラクタから呼ばれるかもしれません。サブクラスの関数の実装ではなく、基本クラスの実装が呼び出されます。したがって、ここで仮想関数を呼び出すことは実際には意味がありません。

しかし、あなたの設計がLiskov Substitutionの原則を満たしているなら、害はありません。おそらくそれが許容される理由です - エラーではなく警告です。


5

他の答えがまだ述べていないこの質問の1つの重要な側面は、コンストラクタ内から仮想クラスを呼び出すことが基本クラスにとって安全であるということです。それが派生クラスが期待していることであれば。そのような場合、派生クラスの設計者は、構築が完了する前に実行されるメソッドが、状況下で可能な限り賢明に振舞うようにする責任があります。例えば、C ++ / CLIでは、コンストラクタは以下を呼び出すコードでラップされています。Dispose構築が失敗した場合は、部分的に構築されたオブジェクトに対して。呼び出しDisposeそのような場合、リソースリークを防ぐためにしばしば必要ですが、Disposeメソッドは、それらが実行されるオブジェクトが完全には構築されていない可能性がある可能性について準備しなければなりません。


4

コンストラクタが実行を完了するまで、オブジェクトは完全にインスタンス化されていません。仮想関数によって参照されるメンバーは初期化されない可能性があります。 C ++では、あなたがコンストラクタの中にいるとき、this自分がいるコンストラクタの静的型のみを参照し、作成中のオブジェクトの実際の動的型は参照しません。これは、仮想関数呼び出しがあなたが期待するところまで行かないかもしれないことを意味します。


3

警告は、仮想メンバーが派生クラスでオーバーライドされる可能性が高いことを思い出させるものです。その場合、親クラスが仮想メンバーに対して行ったことはすべて、子クラスをオーバーライドすることによって元に戻されるか変更されます。明快さのために小さい例打撃を見てください

以下の親クラスは、コンストラクタの仮想メンバーに値を設定しようとします。そしてこれは再シャープ警告を引き起こすでしょう、コードで見てみましょう:

public class Parent
{
    public virtual object Obj{get;set;}
    public Parent()
    {
        // Re-sharper warning: this is open to change from 
        // inheriting class overriding virtual member
        this.Obj = new Object();
    }
}

この子クラスは、親プロパティをオーバーライドします。このプロパティに仮想のマークが付けられていない場合、コンパイラはそのプロパティが親クラスのプロパティを隠していることを警告し、意図的であれば 'new'キーワードを追加することを提案します。

public class Child: Parent
{
    public Child():base()
    {
        this.Obj = "Something";
    }
    public override object Obj{get;set;}
}

最後に使用への影響、以下の例の出力は親クラスのコンストラクタによって設定された初期値を放棄します。そしてこれがRe-sharperがあなたに警告しようとしていることです親クラスのコンストラクタに設定された値は、親クラスのコンストラクタの直後に呼び出される子クラスのコンストラクタによって上書きされることがあります

public class Program
{
    public static void Main()
    {
        var child = new Child();
        // anything that is done on parent virtual member is destroyed
        Console.WriteLine(child.Obj);
        // Output: "Something"
    }
} 


3

盲目的にResharperの助言に従い、クラスを封印することに気をつけてください! EF Code Firstのモデルの場合、virtualキーワードが削除され、関係の遅延ロードが無効になります。

    public **virtual** User User{ get; set; }


3

欠けている重要な点の1つは、この問題を解決する正しい方法は何ですか?

としてグレッグは説明したここでの根本的な問題は、派生クラスが構築される前に基本クラスのコンストラクタが仮想メンバを呼び出すということです。

次のコードは、MSDNのコンストラクタ設計ガイドライン、この問題を示しています。

public class BadBaseClass
{
    protected string state;

    public BadBaseClass()
    {
        this.state = "BadBaseClass";
        this.DisplayState();
    }

    public virtual void DisplayState()
    {
    }
}

public class DerivedFromBad : BadBaseClass
{
    public DerivedFromBad()
    {
        this.state = "DerivedFromBad";
    }

    public override void DisplayState()
    {   
        Console.WriteLine(this.state);
    }
}

の新しいインスタンスがDerivedFromBadが作成されると、基本クラスのコンストラクタはDisplayStateとショーBadBaseClassフィールドは派生コンストラクタによってまだ更新されていないためです。

public class Tester
{
    public static void Main()
    {
        var bad = new DerivedFromBad();
    }
}

改良された実装は、基本クラスのコンストラクタから仮想メソッドを削除して、Initialize方法。の新しいインスタンスを作成するDerivedFromBetter予想される "DerivedFromBetter"を表示します

public class BetterBaseClass
{
    protected string state;

    public BetterBaseClass()
    {
        this.state = "BetterBaseClass";
        this.Initialize();
    }

    public void Initialize()
    {
        this.DisplayState();
    }

    public virtual void DisplayState()
    {
    }
}

public class DerivedFromBetter : BetterBaseClass
{
    public DerivedFromBetter()
    {
        this.state = "DerivedFromBetter";
    }

    public override void DisplayState()
    {
        Console.WriteLine(this.state);
    }
}


  • ええと、私はDerivedFromBetterコンストラクタはBetterBaseClassコンストラクタの暗黙性を呼び出すと思います。上記のコードはpublic DerivedFromBetter():base()と同等である必要があります。そのため、初期化は2回呼び出されます。 - user1778606
  • 保護されたコンストラクタをBetterBaseClassクラスに定義することができます。bool initializeパラメータ。Initialize基本コンストラクターで呼び出されます。派生コンストラクタは、次に呼び出すでしょうbase(false)Initializeを2回呼び出さないでください。 - Sven Vranckx
  • @ user1778606:絶対に!あなたの観察でこれを修正しました。ありがとうございます。 - Gustavo Mori

1

この特定のケースでは、C ++とC#の間に違いがあります。 C ++では、オブジェクトは初期化されていないため、コンストラクタ内で仮想関数を呼び出すのは危険です。 C#では、クラスオブジェクトが作成されると、そのすべてのメンバはゼロで初期化されます。コンストラクタ内で仮想関数を呼び出すことは可能ですが、それでもゼロであるメンバにアクセスする可能性がある場合はそうです。メンバーにアクセスする必要がない場合は、C#で仮想関数を呼び出しても安全です。


  • C ++でコンストラクタ内で仮想関数を呼び出すことは禁止されていません。 - qbeuek
  • C ++についても同じことが言えます。メンバーにアクセスする必要がないのであれば、メンバーが初期化されていなくても問題ありません... - David Pierre
  • いいえ。C++のコンストラクタで仮想メソッドを呼び出すと、最もオーバーライドされた実装ではなく、現在の型に関連付けられているバージョンが呼び出されます。仮想的に呼ばれますが、あたかも現在のクラスの型のように - 派生クラスのメソッドやメンバーにアクセスすることはできません。 - qbeuek

1

私の考えを加えるためだけに。プライベートフィールドを定義するときに常に初期化する場合は、この問題を回避する必要があります。少なくとも以下のコードは魅力のように動作します。

class Parent
{
    public Parent()
    {
        DoSomething();
    }
    protected virtual void DoSomething()
    {
    }
}

class Child : Parent
{
    private string foo = "HELLO";
    public Child() { /*Originally foo initialized here. Removed.*/ }
    protected override void DoSomething()
    {
        Console.WriteLine(foo.ToLower());
    }
}


  • コンストラクタにステップインしたい場合は、デバッグが多少難しくなるため、これを行うことはほとんどありません。 - Phil1970

0

私が見つけたもう一つの興味深いことはReSharperエラーが私にはつまらない以下のような何かをすることによって「満足」されることができるということです。

public class ConfigManager
{

   public virtual int MyPropOne { get; private set; }
   public virtual string MyPropTwo { get; private set; }

   public ConfigManager()
   {
    Setup();
   }

   private void Setup()
   {
    MyPropOne = 1;
    MyPropTwo = "test";
   }

}


  • あなたは回避策を見つけるべきではありませんが実際の問題を解決するべきです。 - alzaimar
  • 私は@ alzaimarに同意します!私は、同様の問題に直面している人や上記の解決策を実行したくない人のために選択肢を残そうとしています。これについて(私の上記の回避策で述べたように)、私が指摘しているもう1つのことは、可能であればReSharperもこの回避策をエラーとしてフラグを立てることができる必要があるということです。しかし、それは現在2つのことにつながる可能性があります - 彼らはこのシナリオを忘れているか、彼らは今考えられないいくつかの有効なユースケースのために故意にそれを除外したかった。 - adityap

-1

Initialize()メソッドを基本クラスに追加してから、派生コンストラクタから呼び出します。そのメソッドは、すべてのコンストラクタが実行された後で、仮想/抽象メソッド/プロパティを呼び出します。


  • これで警告は消えますが、問題は解決しません。派生クラスを追加すると、他の人が説明したのと同じ問題に遭遇します。 - Stefan Bormann

リンクされた質問


関連する質問

最近の質問