PVS-Studioを使用したFlashDevelopソースコードの確認

静的アナライザーとその広告の診断の品質を確認するために、オープンソースプロジェクトを定期的に分析しています。 FlashDevelopプロジェクトの開発者自身が製品を確認するように依頼しましたが、喜んでそれを行いました。

はじめに

FlashDevelopは、Action Scriptバージョン2および3、Haxe、JavaScript、HTML、PHP、C＃をサポートする人気のあるFlashアプリケーション開発環境であり、コード補完、svn、git、mercurial、テンプレートの組み込みサポートなど、最新のコードエディターに固有の機能を備えています、サードパーティのプラグイン、構文強調テーマなど。 特に、FlashDevelopはFireaxis Gamesを使用してXCOM：Enemy Unknownを開発しました。

検証結果

FlashDevelopはオープンソース製品であり、C＃で記述されていることを考慮して、アナライザーでテストしたいと考えました。 分析には、PVS-Studio v6.05静的アナライザーを使用しました。 この記事のフレームワーク内で見つかったすべての問題領域を分析することはできないため、最も興味深いアナライザーメッセージを検討します。

未使用のメソッドの戻り値

ご存知のように、C＃の文字列は不変オブジェクトであり、文字列を変更するメソッドは、元の文字列を変更せずに、実際にString型の新しいオブジェクトを返します。 ただし、実践が示すように、開発者はこの機能を忘れています。 たとえば、アナライザーは次のエラーを検出しました。

V3010関数「Insert」の戻り値を使用する必要があります。 ASPrettyPrinter.cs 1263

public void emit(IToken tok) { .... lineData.Insert(0, mSourceData.Substring(prevLineEnd, ((CommonToken)t).StartIndex - prevLineEnd)); .... } 

V3010関数「Insert」の戻り値を使用する必要があります。 MXMLPrettyPrinter.cs 383

 private void prettyPrint(....) { .... while (aToken.Line == currentLine) { lineData.Insert(0, aToken.Text); .... } .... } 

おそらく、開発者はこの設計を念頭に置いていました。

 lineData = lineData.Insert(....); 

V3010診断トリガーの別の例：

V3010関数 'NextDouble'の戻り値を使用する必要があります。 ASFileParser.cs 196

 private static string getRandomStringRepl() { random.NextDouble(); return "StringRepl" + random.Next(0xFFFFFFF); } 

このコードには機能の観点からのエラーは含まれていませんが、 random.NextDouble（）の呼び出しには意味的な負荷はなく、削除できます。

型キャスト後のNULLチェック

元の型を目的の型にキャストできない場合に、結果の値のnullを確認することは、型変換操作の後の標準的な方法です。 このような日常的な操作を実行する場合、開発者は不注意で、間違った変数をチェックする可能性があります。 当社のアナライザーは飽きず、そのようなことを注意深く監視します。

V3019 「as」キーワードを使用した型変換後に、誤った変数がnullと比較される可能性があります。 変数「item」、「val」を確認してください。 WizardHelper.cs 67

 public static void SetControlValue(....) { .... string val = item as string; if (item == null) continue; .... } 

明らかに、この例では、 itemではなくnull変数をチェックする必要があり、コードは次のようになります。

 string val = item as string; if (val == null) continue; 

メソッド本体の複製

同一のボディを持つメソッドがコード内で見つかった場合、これは常に疑念を引き起こします。 最良の場合、そのようなコードはリファクタリングを必要とし、最悪の場合、機械的なコピー＆ペーストはプログラムのロジックを歪めます。 根拠がないようにするために、次の例を検討してください。

V3013 「SuspendMdiClientLayout」関数の本体が「PerformMdiClientLayout」関数の本体と完全に同等であることは奇妙です（377、行389）。 DockPanel.MdiClientController.cs 377

 private void SuspendMdiClientLayout() { if (GetMdiClientController().MdiClient != null) GetMdiClientController().MdiClient.PerformLayout(); //<= } private void PerformMdiClientLayout() { if (GetMdiClientController().MdiClient != null) GetMdiClientController().MdiClient.PerformLayout(); } 

ご覧のとおり 、 SuspendMdiClientLayoutメソッドとPerformMdiClientLayoutメソッドの本体はまったく同じです。 これはおそらくコードのコピーが原因でした。 SuspendMdiClientLayoutメソッドの名前は、レイアウトを一時停止することを前提としていますが、代わりにレイアウトが再描画されます： MdiClient.PerformLayout（） 。 このメソッドの正しい実装は次のようになっていると思います。

 private void SuspendMdiClientLayout() { if (GetMdiClientController().MdiClient != null) GetMdiClientController().MdiClient.SuspendLayout(); //<= } 

別の例。 このプロジェクトは、何かの字句解析のために設計されたLexerタイプを実装します。 このタイプは、 プライベートスタティックブールStateXX（FsmContext ctx）の形式のシグネチャを持つ同じタイプのメソッドを28個実装します。XXは1〜28の範囲にあります。エラーコード内で、PVS-Studioアナライザーは次のように反応します。

V3013 「State11」関数の本体が「State15」関数の本体と完全に同等であることは奇妙です（532、589行目）。 Lexer.cs 532

 private static bool State11 (FsmContext ctx) { ctx.L.GetChar (); switch (ctx.L.input_char) { case 'e': ctx.Return = true; ctx.NextState = 1; return true; default: return false; } } private static bool State15 (FsmContext ctx) { ctx.L.GetChar (); switch (ctx.L.input_char) { case 'e': ctx.Return = true; ctx.NextState = 1; return true; default: return false; } } 

2つの方法が同じ状況を処理するという事実は非常に疑わしいようです。 この問題を修正する方法は明確ではありません;作業の論理は著者のみが知っています。 また、大量の単調なコードを読むのは書くよりもはるかに難しいので、この問題はコードレビュー中に簡単に検出できることを非常に疑います。 一方、静的アナライザーはここで非常に役立ちます。

サイクルからの無条件の終了

さらに分析すると、このような興味深い点が明らかになりました。

V3020ループ内の無条件の「ブレーク」。 AirWizard.cs 1760

 private void ExtensionBrowseButton_Click(....) { .... foreach (var existingExtension in _extensions) { if (existingExtension.ExtensionId == extensionId) extension = existingExtension; break; } .... } 

開発者が_extensionsコレクションの要素を実行し、対応するextensionIdを持つ最初のexistingExtensionオブジェクトを見つけて、ループを終了することを提案しようと思います。 しかし、括弧の節約により、サイクルは最初の反復後に確実に終了し、プログラムのロジックに大きな影響を与えます。

式は常にtrue / falseです

別の一般的なエラーの原因は条件式です。 式に多数の変数、境界値、かなり複雑な分岐が含まれている場合、エラーが発生する可能性が高くなります。 たとえば、次のような条件演算子を考えてみましょう。

 private void SettingChanged(string setting) { if (setting == "ExcludedFileTypes" || setting == "ExcludedDirectories" || setting == "ShowProjectClasspaths" || setting == "ShowGlobalClasspaths" || setting == "GlobalClasspath") { Tree.RebuildTree(); } else if (setting == "ExecutableFileTypes") { FileInspector.ExecutableFileTypes = Settings.ExecutableFileTypes; } else if (setting == "GlobalClasspath") //<= { // clear compile cache for all projects FlexCompilerShell.Cleanup(); } } 

PVS-Studio静的アナライザーは次のエラーを報告します。

V3022式 'setting == "GlobalClasspath"'は常にfalseです。 PluginMain.cs 1194

実際、 else if条件（設定==“ GlobalClasspath”）は、最初のifに同じ条件が存在するため、決して満たされることはありません。 ただし、一部のロジックの実現は、この条件の実現に依存します。 このメソッドの読みやすさを簡素化するために、 switchステートメントを使用して書き換えます。

不可能な状態の次の例：

V3022式 'high == 0xBF'は常にfalseです。 JapaneseContextAnalyser.cs 293

 protected override int GetOrder(byte[] buf, int offset, out int charLen) { byte high = buf[offset]; //find out current char's byte length if (high == 0x8E || high >= 0xA1 && high <= 0xFE) charLen = 2; else if (high == 0xBF) charLen = 3; .... } 

アナライザーは、式'high == 0xBF'が常にfalseであることを示します。 これは、値0xBFがhigh> = 0xA1 && high <= 0xFEの範囲に該当し、最初のifでチェックされるためです 。

V3022診断からのメッセージの別の例：

V3022式 '！Outline.FlagTestDrop'は常にtrueです。 DockPanel.DockDragHandler.cs 769

 private void TestDrop() { Outline.FlagTestDrop = false; .... if (!Outline.FlagTestDrop) { .... } .... } 

このスニペットでは、falseに設定され、コード内でそれ以上変更されないOutline.FlagTestDropフィールドがif条件ステートメントで使用されていることがわかります。 おそらく、このメソッドでは、開発者がif（！Outline.FlagTestDrop）checkを実装したため、このフィールドの値を変更する機能は実装されていません。

nullがチェックされる前にインスタンスを使用する

実際には、たとえば、型キャスト、コレクションからの要素の選択などの後、nullの変数をチェックする必要が常に発生します。 そのような状況では、結果の変数がnullでないことを確認してから、それを使用します。 ただし、私たちの実践が示すように、開発者は結果のオブジェクトをすぐに使用し始め、それがnullでないことを確認するだけです。 これらのエラーは、V3095診断によって報告されます。

V3095 「node」オブジェクトは、nullに対して検証される前に使用されました。 チェック行：364、365。ProjectContextMenu.cs 364

 private void AddFolderItems(MergableMenu menu, string path) { .... DirectoryNode node = projectTree.SelectedNode as DirectoryNode; if (node.InsideClasspath == node) menu.Add(RemoveSourcePath, 2, true); else if (node != null && ....) { menu.Add(AddSourcePath, 2, false); } .... } 

この例では、 projectTree.SelectedNodeフィールドのタイプはGenericNodeであり、これはDirectoryNodeの基本タイプです。 基本型オブジェクトの派生型へのキャストが失敗し、その結果、ノード変数に空の参照が含まれる場合があります。 ただし、ご覧のとおり 、型変換操作の後、開発者はすぐにnode.InsideClasspathフィールドにアクセスし、 ノード変数のnullのみをチェックします。 このような実装では、NullReferenceExceptionが発生する場合があります。

渡された引数の値を上書きします

アナライザーは、このような潜在的に問題のあるコード内の場所を検出しました。

V3061パラメーター 'b'は、使用される前に常にメソッド本体で書き換えられます。 InBuffer.cs 56

 public bool ReadByte(byte b) // check it { if (m_Pos >= m_Limit) if (!ReadBlock()) return false; b = m_Buffer[m_Pos++]; //<= return true; } 

このメソッドに渡された引数bの値は使用されないため、上書きされ、それでも使用されません。 このメソッドは意図したとおりに実装されていないと想定できます（コメント「 // check it 」もこれを示唆しています）。 おそらく、このメソッドのシグネチャは次のようになります。

 public bool ReadByte(ref byte b) { .... } 

メソッドに渡される引数の順序が正しくありません

アナライザーによって検出された次の疑わしい場所は、コードレビューを実施する際に簡単に気付くことができません。

V3066 「_channelMixer_OVERLAY」メソッドに渡される引数の誤った順序の可能性：「back」および「fore」。 BBCodeStyle.cs 302

 private static float _channelMixer_HARDLIGHT(float back, float fore) { return _channelMixer_OVERLAY(fore, back); } 

_channelMixer_OVERLAYメソッドには次のシグネチャがあります。

 static float _channelMixer_OVERLAY(float back, float fore) 

おそらく、これはまさに意図したものです。 ただし、このメソッドを参照するときに、 fore引数とback引数が逆になっている可能性があります。 そして、アナライザはそのような場所をチェックするのに役立ちます。

安全でないイベントハンドラー呼び出し

V3083診断は 、イベントハンドラーに対する潜在的に安全でない呼び出しを検出するように設計されています。 分析されたプロジェクトでは、この診断により、このような場所が多数明らかになりました。 特定の例を使用して、安全でないハンドラー呼び出しの状況を分析します。

V3083イベント「OnKeyEscape」の安全でない呼び出し、NullReferenceExceptionが発生する可能性があります。 イベントを呼び出す前に、ローカル変数にイベントを割り当てることを検討してください。 QuickFind.cs 849

 protected void OnPressEscapeKey() { if (OnKeyEscape != null) OnKeyEscape(); } 

一見、コードは完全に正しいようです。OnKeyEscapeフィールドがnullでない場合、このイベントを発生させます。 ただし、このアプローチは推奨されません。 OnKeyEscapeイベントに1つのサブスクライバーがあると仮定し、このフィールドでnullをチェックした後、このサブスクライバーが（たとえば、別のスレッドで）このイベントからサブスクライブ解除したとします。 イベントにサブスクライバーがない場合、OnKeyEscapeフィールドには空のリンクが含まれ、イベントを発生させようとするとNullReferenceExceptionが発生します。

これが非常に難しい再現可能なエラーであることは特に不快です。 ユーザーは、ESCを押すとエラーが発生したと不平を言うかもしれません。 ただし、ESCを1000回押しても、プログラマが何が間違っているのかを理解できるとは考えられません。

追加の中間変数を宣言することにより、イベント呼び出しを保護できます。

 var handler = OnKeyEscape if (handler != null) handler(); 

C＃バージョン6では、null（？。）Checkステートメントが表示され、コードを大幅に簡素化できます。

 OnKeyEscape?.Invoke(); 

考えられるタイプミス

アナライザーのヒューリスティック機能により、コード内の非常に興味深い疑わしい場所を検出できます。 例：

V3056 「a1」アイテムの使用の正確さを検討することを検討してください。 LzmaEncoder.cs 225

 public void SetPrices(....) { UInt32 a0 = _choice.GetPrice0(); UInt32 a1 = _choice.GetPrice1(); UInt32 b0 = a1 + _choice2.GetPrice0(); UInt32 b1 = a1 + _choice2.GetPrice1(); .... } 

このコードはコピーアンドペーストを使用して書かれた可能性があります。 また、変数b0の値を計算するには、変数a1の代わりに変数a0を使用する必要があるように思えます。 いずれにせよ、発見された疑わしい場所は、開発者がこのコードを注意深く見る機会であるはずです。 一般に、より有益な変数名を使用する方が適切です。

再び例外を投げる

キャッチされた例外がさらにスローされるコード内のいくつかの場所が見つかりました。 これは、たとえば次のように実装されます。

 public void Copy(string fromPath, string toPath) { .... try { .... } catch (UserCancelException uex) { throw uex; } .... } 

このメソッドをチェックすると、アナライザーは次のメッセージを表示します。

V3052元の例外オブジェクト「uex」が飲み込まれました。 元の例外のスタックが失われる可能性があります。 FileActions.cs 598

このような例外のスローは、元の呼び出しスタックが現在のメソッドで始まる新しいものによって上書きされるという事実につながります。 これにより、デバッグ中に元の例外が発生した場所を見つけることが非常に難しくなります。

再び例外をスローするときに元の呼び出しスタックを保持するには、 throwステートメントを使用するだけです。

 try { .... } catch (UserCancelException uex) { throw; } 

コレクションを走査するときに発生する可能性のあるInvalidCastException

コードをさらに分析すると、潜在的に安全でない場所が明らかになりました。

V3087 「foreach」で列挙された変数の型は、コレクションの要素の型にキャスト可能であるとは限りません。 VS2005DockPaneStrip.cs 1436

 private void WindowList_Click(object sender, EventArgs e) { .... List<Tab> tabs = new List<Tab>(Tabs); foreach (TabVS2005 tab in tabs) .... } 

tabsコレクションにはTab型の要素が含まれますが、反復中にこのコレクションの要素はTab型の子孫であるTabVS2005型にキャストされます。 このキャストは安全ではないため、 System.InvalidCastExceptionがスローされる場合があります。

同じ診断で同様の安全でないコードが検出されました：

 public int DocumentsCount { get { int count = 0; foreach (DockContent content in Documents) count++; return count; } } 

ここで、 DocumentsコレクションにはIDockContent要素が含まれており、 DockContent型への明示的な変換は安全ではない場合があります。

過度の条件付きチェック

最後に、エラーを含まないが、それでも非常に複雑なコードの例を見てみましょう。

V3031過剰なチェックを簡素化できます。 「||」 演算子は反対の式で囲まれています。 DockContentHandler.cs 540

 internal void SetDockState(....) { .... if ((Pane != oldPane) || (Pane == oldPane && oldDockState != oldPane.DockState)) { RefreshDockPane(Pane); } .... } 

条件Pane！= OldPaneとPane == oldPaneは相互に排他的であるため、この式は単純化できます。

 if (Pane != oldPane || oldDockState != oldPane.DockState) 

同様に、別のメソッドの条件式：

 void SetProject(....) { .... if (!internalOpening || (internalOpening && !PluginBase.Settings.RestoreFileSession)) { RestoreProjectSession(project); } .... } 

次のように簡略化することもできます。

 if (!internalOpening || !PluginBase.Settings.RestoreFileSession) 

おわりに

FlashDevelopプロジェクトは10年以上開発されており、かなり大きなコードベースを持っています。 このようなプロジェクトで静的コードアナライザーを使用すると、興味深い結果が得られ、ソフトウェア製品の品質が向上します。 プロジェクトの開発者が見つけたエラーを見ることは興味深いと思います。 C、C ++、またはC＃のすべての開発者は、最新バージョンのPVS-Studio静的コードアナライザーをダウンロードして 、プロジェクトを確認することをお勧めします。

試用版では不十分な場合（ 詳細 ）、私たちに連絡して、ツールのより詳細な研究の鍵を取得することをお勧めします。


この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Pavel Kusnetsov。 PVS-Studioを使用したFlashDevelopのソースコードの確認 。