diff --git a/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpression.cs b/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpression.cs index 72ba1c79..2889137e 100644 --- a/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpression.cs +++ b/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpression.cs @@ -120,6 +120,31 @@ public bool Evaluate(CodeElement? item) return _conditions.Any(c => c.Evaluate(item)); } } + + /// + /// Negates a condition, so a search can exclude instead of select ("-Strings." hides everything + /// whose name contains "Strings."). + /// + /// An item without a code element never matches, not even a negated condition. Every term + /// answers "no" for a null item, and negation must not silently turn that into a match - the + /// tree has a virtual root without a code element that would otherwise light up on every + /// exclusion. + /// + /// + internal class Not : IExpression + { + private readonly IExpression _condition; + + public Not(IExpression condition) + { + _condition = condition; + } + + public bool Evaluate(CodeElement? item) + { + return item is not null && !_condition.Evaluate(item); + } + } } internal class FullNameSearch(string searchTerm) : Term(searchTerm) diff --git a/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpressionFactory.cs b/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpressionFactory.cs index 21d57a83..b01a620e 100644 --- a/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpressionFactory.cs +++ b/CSharpCodeAnalyst.AnalyzerSdk/Search/SearchExpressionFactory.cs @@ -2,6 +2,13 @@ public static class SearchExpressionFactory { + /// + /// Marks a term as excluding rather than selecting. A minus sign cannot start a C# identifier, so + /// it is free to use here; imported graphs may contain one inside a name, but not at the start of + /// a search term. + /// + private const char NegationPrefix = '-'; + private static Term CreateTerm(string search, TextSearchField searchField) { if (searchField == TextSearchField.FullName) @@ -12,7 +19,29 @@ private static Term CreateTerm(string search, TextSearchField searchField) return new NameSearch(search); } - public static IExpression CreateSearchExpression(string searchText, TextSearchField searchField = TextSearchField.FullName) + /// + /// Wraps the term in a negation when it starts with '-'. The negation belongs to its own term, so + /// it binds tighter than the AND of a group and than the OR between groups: "-a b | c" reads as + /// "((NOT a) AND b) OR c". A lone '-' has nothing to negate and stays a literal search term. + /// + private static IExpression CreateTermOrNegation(string token, TextSearchField searchField, bool allowNegation) + { + if (allowNegation && token.Length > 1 && token[0] == NegationPrefix) + { + return new Term.Not(CreateTerm(token[1..], searchField)); + } + + return CreateTerm(token, searchField); + } + + /// + /// Whether a leading '-' excludes the term. Pass false where an expression that matches almost + /// everything is harmful rather than useful: the tree expands and highlights every ancestor of a + /// match, so an exclusion would unfold the whole tree at once. With negation off the '-' is part + /// of the search term like any other character. + /// + public static IExpression CreateSearchExpression(string searchText, + TextSearchField searchField = TextSearchField.FullName, bool allowNegation = true) { // Or binds less. var orTerms = searchText @@ -24,7 +53,7 @@ public static IExpression CreateSearchExpression(string searchText, TextSearchFi { var andExpressions = orTerm .Split([' '], StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) - .Select(IExpression (t) => CreateTerm(t, searchField)) + .Select(t => CreateTermOrNegation(t, searchField, allowNegation)) .ToArray(); orExpressions.Add(new Term.And(andExpressions)); diff --git a/CSharpCodeAnalyst.Analyzers/DeadCode/Analyzer.cs b/CSharpCodeAnalyst.Analyzers/DeadCode/Analyzer.cs new file mode 100644 index 00000000..f6549255 --- /dev/null +++ b/CSharpCodeAnalyst.Analyzers/DeadCode/Analyzer.cs @@ -0,0 +1,69 @@ +using CSharpCodeAnalyst.Analyzers.DeadCode.Presentation; +using CSharpCodeAnalyst.Analyzers.Resources; +using CSharpCodeAnalyst.AnalyzerSdk.Contracts; +using CSharpCodeAnalyst.AnalyzerSdk.Messages; +using CSharpCodeAnalyst.AnalyzerSdk.Notifications; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; +using CSharpCodeAnalyst.CodeGraph.Declarations; + +namespace CSharpCodeAnalyst.Analyzers.DeadCode; + +/// +/// Lists the code nobody references any more - the topmost element of every dead subtree, together +/// with the hint that explains why it might still be alive. +/// +public class Analyzer : IAnalyzer +{ + private readonly ExternalContractStore _externalContracts; + private readonly IPublisher _messaging; + private readonly IUserNotification _userNotification; + + public Analyzer(IPublisher messaging, IUserNotification userNotification, + ExternalContractStore externalContracts) + { + _messaging = messaging; + _userNotification = userNotification; + _externalContracts = externalContracts; + } + + public string Id { get; } = "DeadCode"; + public string Name { get; } = Strings.Analyzer_DeadCode_Label; + public string Description { get; set; } = Strings.Analyzer_DeadCode_Tooltip; + + public void Analyze(CodeGraph.Graph.CodeGraph graph) + { + var findings = DeadCodeAnalysis.Calculate(graph, _externalContracts); + + if (findings.Count == 0) + { + _userNotification.ShowSuccess(Strings.Analyzer_DeadCode_NoData); + return; + } + + var vm = new DeadCodeViewModel(findings, _messaging); + _messaging.Publish(new ShowTabularDataRequest(Id, Name, vm)); + } + + public string? GetPersistentData() + { + // No configuration or state to persist. + return null; + } + + public void SetPersistentData(string? data) + { + // No configuration or state to persist. + } + + public bool IsDirty() + { + return false; + } + + public event EventHandler? DataChanged; + + protected virtual void OnDataChanged() + { + DataChanged?.Invoke(this, EventArgs.Empty); + } +} diff --git a/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeRowViewModel.cs b/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeRowViewModel.cs new file mode 100644 index 00000000..94f999c8 --- /dev/null +++ b/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeRowViewModel.cs @@ -0,0 +1,97 @@ +using CSharpCodeAnalyst.Analyzers.Resources; +using CSharpCodeAnalyst.AnalyzerSdk.DynamicDataGrid.Contracts.TabularData; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CSharpCodeAnalyst.Analyzers.DeadCode.Presentation; + +public class DeadCodeRowViewModel : TableRow +{ + /// Beyond this many related members the hint only states the count - the cell has to stay readable. + private const int MaxNamedRelatedMembers = 3; + + internal DeadCodeRowViewModel(DeadCodeFinding finding) + { + Element = finding.Element; + Name = finding.Element.FullName; + Kind = finding.Element.ElementType.ToString(); + // Fully qualified: WPF pulls a global "Accessibility" namespace into scope; ours is AccessLevel. + Access = finding.Element.AccessLevel == CodeGraph.Graph.AccessLevel.Unknown + ? string.Empty + : finding.Element.AccessLevel.ToString(); + + Confidence = finding.Confidence.ToString(); + + // Bound for the colour rating and for sorting; the column displays the word. + ConfidenceValue = (int)finding.Confidence; + Hint = FormatHint(finding); + } + + /// The underlying graph node, used to jump to the source and to add it to the Code Explorer. + public CodeElement Element { get; } + + public string Name { get; } + public string Kind { get; } + + /// The element's visibility, empty when the producer did not supply one. + public string Access { get; } + + public string Confidence { get; } + + /// Numeric backer of for the colour rating and for sorting. + public int ConfidenceValue { get; } + + /// + /// Two kinds of note, joined into one cell: why the element might be alive despite having no + /// visible reference (entry point, test code, attributes), and - for a contract finding - what + /// dies together with it. Empty means neither applies, so nothing speaks against deleting it. + /// + public string Hint { get; } + + private static string FormatHint(DeadCodeFinding finding) + { + var parts = new List(); + + if (finding.Hints.HasFlag(DeadCodeHint.EntryPoint)) + { + parts.Add(Strings.DeadCode_Hint_EntryPoint); + } + + if (finding.Hints.HasFlag(DeadCodeHint.TestCode)) + { + parts.Add(Strings.DeadCode_Hint_TestCode); + } + + if (finding.Hints.HasFlag(DeadCodeHint.ContractNeverCalled)) + { + parts.Add(string.Format(Strings.DeadCode_Hint_ContractNeverCalled, FormatRelated(finding))); + } + + if (finding.Hints.HasFlag(DeadCodeHint.ImplementsDeadContract)) + { + parts.Add(string.Format(Strings.DeadCode_Hint_ImplementsDeadContract, FormatRelated(finding))); + } + + if (finding.Hints.HasFlag(DeadCodeHint.ImplementsExternalContract)) + { + parts.Add(string.Format(Strings.DeadCode_Hint_ImplementsExternalContract, finding.ExternalContract)); + } + + if (finding.Hints.HasFlag(DeadCodeHint.Attributed)) + { + parts.Add(string.Format(Strings.DeadCode_Hint_Attributed, string.Join(", ", finding.Attributes))); + } + + return string.Join("; ", parts); + } + + private static string FormatRelated(DeadCodeFinding finding) + { + if (finding.RelatedMembers.Count > MaxNamedRelatedMembers) + { + return string.Format(Strings.DeadCode_Hint_RelatedCount, finding.RelatedMembers.Count); + } + + return string.Join(", ", finding.RelatedMembers.Select(m => m.FullName)); + } +} diff --git a/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeViewModel.cs b/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeViewModel.cs new file mode 100644 index 00000000..6c998803 --- /dev/null +++ b/CSharpCodeAnalyst.Analyzers/DeadCode/Presentation/DeadCodeViewModel.cs @@ -0,0 +1,141 @@ +using System.Collections.ObjectModel; +using System.Windows; +using CSharpCodeAnalyst.Analyzers.Resources; +using CSharpCodeAnalyst.AnalyzerSdk.Contracts; +using CSharpCodeAnalyst.AnalyzerSdk.DynamicDataGrid.Contracts.TabularData; +using CSharpCodeAnalyst.AnalyzerSdk.Messages; +using CSharpCodeAnalyst.AnalyzerSdk.Search; +using CSharpCodeAnalyst.AnalyzerSdk.Wpf; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; + +namespace CSharpCodeAnalyst.Analyzers.DeadCode.Presentation; + +internal class DeadCodeViewModel : Table +{ + private readonly IPublisher _messaging; + private readonly ObservableCollection _rows; + + internal DeadCodeViewModel(List findings, IPublisher messaging) + { + _messaging = messaging; + + // Highest confidence first: that is the part of the result you can work through without checking + // every entry by hand, so it belongs at the top before anyone touches a column header. The + // analysis already sorts by name, which stays the tie breaker within a confidence band. + var rows = findings + .OrderByDescending(f => f.Confidence) + .Select(f => new DeadCodeRowViewModel(f)); + _rows = new ObservableCollection(rows); + } + + public override bool CanFilter => true; + + public override IEnumerable GetColumns() + { + return new List + { + new() + { + Type = ColumnType.Text, + Header = Strings.Column_DeadCode_Element, + PropertyName = nameof(DeadCodeRowViewModel.Name) + }, + new() + { + Type = ColumnType.Text, + Header = Strings.Column_DeadCode_Kind, + PropertyName = nameof(DeadCodeRowViewModel.Kind), + Width = 90 + }, + new() + { + Type = ColumnType.Text, + Header = Strings.Column_DeadCode_Access, + PropertyName = nameof(DeadCodeRowViewModel.Access), + Width = 80 + }, + new() + { + Type = ColumnType.Text, + Header = Strings.Column_DeadCode_Confidence, + PropertyName = nameof(DeadCodeRowViewModel.Confidence), + Width = 80, + + // High (2) green, Medium (1) orange, Low (0) red - here a larger value is better. + Rating = new ThresholdRating(2, 1, false), + RatingValuePropertyName = nameof(DeadCodeRowViewModel.ConfidenceValue), + SortMemberName = nameof(DeadCodeRowViewModel.ConfidenceValue) + }, + new() + { + // Carries both the doubts (entry point, test code, attributes) and the explanation of a + // contract finding. Empty means nothing speaks against deleting the element, and sorting + // brings those rows together. + Type = ColumnType.Text, + Header = Strings.Column_DeadCode_Hint, + PropertyName = nameof(DeadCodeRowViewModel.Hint) + } + }; + } + + public override ObservableCollection GetData() + { + return _rows; + } + + /// + /// Filters by element name using the same search expression as the Advanced Search (camel-case, + /// OR via '|', AND via spaces, exclusion via a leading '-'). Exclusion is what makes a long result + /// usable: "-Strings. -Tests" drops whole groups of findings at once. + /// + public override ObservableCollection Filter(string searchText) + { + if (string.IsNullOrWhiteSpace(searchText)) + { + return _rows; + } + + var expression = SearchExpressionFactory.CreateSearchExpression(searchText); + var filtered = _rows + .Cast() + .Where(row => expression.Evaluate(row.Element)); + return new ObservableCollection(filtered); + } + + public override DataTemplate? GetRowDetailsTemplate() + { + return null; + } + + public override List GetCommands() + { + return + [ + new CommandDefinition + { + Header = Strings.JumpToCode, + Command = new WpfCommand(JumpToCode, CanJumpToCode) + }, + new CommandDefinition + { + Header = Strings.CopyToExplorerGraph_MenuItem, + Command = new WpfCommand(ShowInExplorer) + } + ]; + } + + private void ShowInExplorer(DeadCodeRowViewModel row) + { + _messaging.Publish(new AddNodeToGraphRequest(row.Element)); + } + + private static bool CanJumpToCode(DeadCodeRowViewModel row) + { + return row.Element.SourceLocations.Count > 0; + } + + private void JumpToCode(DeadCodeRowViewModel row) + { + _messaging.Publish(new OpenSourceLocationRequest(row.Element.SourceLocations[0])); + } +} diff --git a/CSharpCodeAnalyst.Analyzers/Resources/Strings.Designer.cs b/CSharpCodeAnalyst.Analyzers/Resources/Strings.Designer.cs index cd584a66..5aadd87b 100644 --- a/CSharpCodeAnalyst.Analyzers/Resources/Strings.Designer.cs +++ b/CSharpCodeAnalyst.Analyzers/Resources/Strings.Designer.cs @@ -257,6 +257,141 @@ public static string Analyzer_SystemMetrics_Tooltip { } } + /// + /// Looks up a localized string similar to Dead Code. + /// + public static string Analyzer_DeadCode_Label { + get { + return ResourceManager.GetString("Analyzer_DeadCode_Label", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to No unreferenced elements found. + /// + public static string Analyzer_DeadCode_NoData { + get { + return ResourceManager.GetString("Analyzer_DeadCode_NoData", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Finds elements nothing references any more. Only the topmost element of a dead subtree is listed; the hint column marks the cases that may still be used through XAML, reflection or a test runner.. + /// + public static string Analyzer_DeadCode_Tooltip { + get { + return ResourceManager.GetString("Analyzer_DeadCode_Tooltip", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Element. + /// + public static string Column_DeadCode_Element { + get { + return ResourceManager.GetString("Column_DeadCode_Element", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Notes. + /// + public static string Column_DeadCode_Hint { + get { + return ResourceManager.GetString("Column_DeadCode_Hint", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Kind. + /// + public static string Column_DeadCode_Kind { + get { + return ResourceManager.GetString("Column_DeadCode_Kind", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Access. + /// + public static string Column_DeadCode_Access { + get { + return ResourceManager.GetString("Column_DeadCode_Access", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Confidence. + /// + public static string Column_DeadCode_Confidence { + get { + return ResourceManager.GetString("Column_DeadCode_Confidence", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Attributes: {0}. + /// + public static string DeadCode_Hint_Attributed { + get { + return ResourceManager.GetString("DeadCode_Hint_Attributed", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Implemented but never called: {0}. + /// + public static string DeadCode_Hint_ContractNeverCalled { + get { + return ResourceManager.GetString("DeadCode_Hint_ContractNeverCalled", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Entry point. + /// + public static string DeadCode_Hint_EntryPoint { + get { + return ResourceManager.GetString("DeadCode_Hint_EntryPoint", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Implements unused contract: {0}. + /// + public static string DeadCode_Hint_ImplementsDeadContract { + get { + return ResourceManager.GetString("DeadCode_Hint_ImplementsDeadContract", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Implements external contract: {0}. + /// + public static string DeadCode_Hint_ImplementsExternalContract { + get { + return ResourceManager.GetString("DeadCode_Hint_ImplementsExternalContract", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to {0} members. + /// + public static string DeadCode_Hint_RelatedCount { + get { + return ResourceManager.GetString("DeadCode_Hint_RelatedCount", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Test code. + /// + public static string DeadCode_Hint_TestCode { + get { + return ResourceManager.GetString("DeadCode_Hint_TestCode", resourceCulture); + } + } + /// /// Looks up a localized string similar to Type Cohesion. /// diff --git a/CSharpCodeAnalyst.Analyzers/Resources/Strings.resx b/CSharpCodeAnalyst.Analyzers/Resources/Strings.resx index 1762903e..76416e44 100644 --- a/CSharpCodeAnalyst.Analyzers/Resources/Strings.resx +++ b/CSharpCodeAnalyst.Analyzers/Resources/Strings.resx @@ -203,6 +203,51 @@ No types found to rank + + + Dead Code + + + Finds elements nothing references any more. Only the topmost element of a dead subtree is listed; the hint column marks the cases that may still be used through XAML, reflection or a test runner. + + + No unreferenced elements found + + + Element + + + Kind + + + Access + + + Confidence + + + Notes + + + Entry point + + + Test code + + + Attributes: {0} + + + Implemented but never called: {0} + + + Implements unused contract: {0} + + + Implements external contract: {0} + + + {0} members Type Cohesion diff --git a/CSharpCodeAnalyst.Analyzers/TypeDependencies/Presentation/TypeDependenciesViewModel.cs b/CSharpCodeAnalyst.Analyzers/TypeDependencies/Presentation/TypeDependenciesViewModel.cs index a33e4232..d76526c9 100644 --- a/CSharpCodeAnalyst.Analyzers/TypeDependencies/Presentation/TypeDependenciesViewModel.cs +++ b/CSharpCodeAnalyst.Analyzers/TypeDependencies/Presentation/TypeDependenciesViewModel.cs @@ -81,7 +81,8 @@ public override ObservableCollection GetData() /// /// Filters by type name using the same search expression as the Advanced Search - /// (supports camel-case, OR via '|', AND via spaces). The searched column is the type name. + /// (camel-case, OR via '|', AND via spaces, exclusion via a leading '-'). The searched column is + /// the type name. /// public override ObservableCollection Filter(string searchText) { diff --git a/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeAnalysis.cs b/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeAnalysis.cs new file mode 100644 index 00000000..9d971bc7 --- /dev/null +++ b/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeAnalysis.cs @@ -0,0 +1,631 @@ +using CSharpCodeAnalyst.CodeGraph.Declarations; +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; + +/// +/// Finds code nobody references any more. +/// +/// The rule is expressed over the subtree, not over the single element: an element is dead when no +/// relationship enters its subtree from the outside. That makes the obvious case work - a class +/// whose method is called from elsewhere is alive even though nothing names the class itself - and +/// it also stops a class from keeping itself alive: methods that only call each other are internal +/// to the subtree and prove nothing. Because a dead element implies a dead subtree, only the +/// topmost dead element of a subtree is reported. +/// +/// +/// Polymorphism is handled by propagating liveness instead of counting the edge as a reference. +/// "Implements" / "Overrides" point from the implementation to the contract, so an implementation +/// never has an incoming reference and a contract member always looks used. Both are wrong. We +/// therefore ignore those edges as references and instead push liveness the other way: a contract +/// member that is called keeps all its implementations (and their types) alive. A contract that is +/// never called dies together with its implementations, which is exactly the finding one wants. +/// Contracts from outside the analyzed code are the exception - we cannot see who calls them, so +/// the implementation is assumed alive. That assumption deliberately does not extend to the +/// containing type: a class whose only "use" is implementing IDisposable is still dead code. +/// +/// +/// The analysis reports exactly what nothing references right now. It does not chase the +/// consequences: code that is only kept alive by the code just reported stays out of the result. +/// That is a deliberate step back from an earlier cascading version, which multiplied every false +/// positive - one invisible XAML binding took seven further elements with it. Deleting a finding +/// and running the analysis again gives the same answer without stacking the uncertainty. +/// +/// +/// Every finding carries a , and +/// is what makes the top level reachable: an element confined to its +/// type or assembly cannot be referenced from code we did not analyze, so "nothing references it" +/// and "nothing can reference it" coincide. A producer that supplies no visibility never reaches +/// that level - which is the honest answer, not a penalty. +/// +/// +/// Two cases are dropped instead of reported: a single property accessor (see +/// ) and a public property of a type marked as a serialization target (see +/// ). Both are the same kind of noise - a getter or setter that +/// only a serializer, a binding or a framework ever touches - and on the affected types they would +/// be the rule rather than the exception. +/// +/// +/// Limitations, by construction: references the parser cannot see (reflection, dependency +/// injection, serialization) look like dead code - see . Dead cycles are +/// not found either: two elements that only reference each other keep each other alive, which needs +/// reachability from an explicit set of entry points rather than a cascade. +/// +/// +public static class DeadCodeAnalysis +{ + /// + /// The notes that say "the caller is somewhere we cannot see". They are what pins a finding to the + /// lowest confidence: we already know we might be wrong about it. + /// + private const DeadCodeHint CallerOutsideTheGraph = + DeadCodeHint.EntryPoint | DeadCodeHint.TestCode | DeadCodeHint.Attributed | + DeadCodeHint.ImplementsExternalContract; + + /// Prefix of the contracts recorded for a type that raises change notifications. + private const string NotifyPropertyChanged = "INotifyPropertyChanged."; + + /// + /// Attribute names (with and without the "Attribute" suffix) of the common test frameworks. A test + /// method is called by a runner, never from the code, so it always looks unreferenced. + /// + private static readonly HashSet TestAttributes = new(StringComparer.OrdinalIgnoreCase) + { + "Test", "TestAttribute", + "TestCase", "TestCaseAttribute", + "TestCaseSource", "TestCaseSourceAttribute", + "TestFixture", "TestFixtureAttribute", + "SetUp", "SetUpAttribute", + "TearDown", "TearDownAttribute", + "OneTimeSetUp", "OneTimeSetUpAttribute", + "OneTimeTearDown", "OneTimeTearDownAttribute", + "Fact", "FactAttribute", + "Theory", "TheoryAttribute", + "TestMethod", "TestMethodAttribute", + "DataTestMethod", "DataTestMethodAttribute", + "TestClass", "TestClassAttribute", + "TestInitialize", "TestInitializeAttribute", + "TestCleanup", "TestCleanupAttribute", + "ClassInitialize", "ClassInitializeAttribute", + "ClassCleanup", "ClassCleanupAttribute", + "Benchmark", "BenchmarkAttribute" + }; + + /// + /// Attribute names (with and without the "Attribute" suffix) that mark a whole type as a + /// serialization target. A serializer reads and writes the public properties by reflection, so on + /// such a type they look unreferenced no matter how heavily the type is used. + /// + private static readonly HashSet SerializationAttributes = new(StringComparer.OrdinalIgnoreCase) + { + "Serializable", "SerializableAttribute", + "DataContract", "DataContractAttribute", + "JsonObject", "JsonObjectAttribute", + "JsonConverter", "JsonConverterAttribute", + "XmlRoot", "XmlRootAttribute", + "XmlType", "XmlTypeAttribute", + "ProtoContract", "ProtoContractAttribute", + "MessagePackObject", "MessagePackObjectAttribute" + }; + + /// + /// What the parser recorded beside the graph: which members implement or override something from + /// outside the analyzed code. Optional - without it those members are reported like any other + /// unreferenced member, which is what they look like from the graph alone. + /// + public static List Calculate(Graph.CodeGraph graph, + ExternalContractStore? externalContracts = null) + { + ArgumentNullException.ThrowIfNull(graph); + + // Element -> the contract outside the analyzed code it implements. Two sources: the store the + // parser fills from the symbols, and - when external code is part of the graph - the edges. + var external = new Dictionary(); + if (externalContracts is not null) + { + foreach (var (elementId, contract) in externalContracts.Contracts) + { + external[elementId] = contract; + } + } + + // Internal contract member -> the members implementing / overriding it, and the reverse. + var implementations = new Dictionary>(); + var contracts = new Dictionary>(); + + // Base type -> the types deriving from it. Only used to spread the binding target property. + var derivedTypes = new Dictionary>(); + + var referenceEdges = new List<(CodeElement Source, CodeElement Target)>(); + CollectEdges(graph, referenceEdges, external, implementations, contracts, derivedTypes); + + var context = new AnalysisContext(external, implementations, contracts, + FindBindingSources(graph, external, derivedTypes), FindSerializableTypes(graph)); + + var referenced = ComputeReferenced(referenceEdges, implementations); + + return Report(graph, referenced, context) + .OrderBy(f => f.Element.FullName, StringComparer.Ordinal) + .ToList(); + } + + /// The structural facts of one run, all derived once from the graph. + private sealed record AnalysisContext( + Dictionary External, + Dictionary> Implementations, + Dictionary> Contracts, + HashSet BindingSources, + HashSet SerializableTypes); + + /// + /// Finds the view models. + /// These are the types whose public properties a XAML {Binding} may read - anything implementing + /// INotifyPropertyChanged. Bindings are resolved by reflection at runtime and are the one + /// XAML construct the parser deliberately does not follow, so such a property must never reach the + /// highest confidence. + /// + /// "Source" in the WPF sense: the object a binding reads from (Binding.Source). The + /// binding target is the dependency property on the control, which is not what we look + /// for here. + /// + /// + /// The interface shows up through the external contract of the PropertyChanged event. + /// A derived view model has no such member of its own (the base class implements it), so the + /// property is spread down the edges - the common + /// "MyViewModel : ViewModelBase" shape would be missed otherwise. A base class outside the + /// analyzed code is invisible here, so a view model deriving from a framework type that + /// implements the interface is not recognized. + /// + /// + private static HashSet FindBindingSources(Graph.CodeGraph graph, Dictionary external, + Dictionary> derivedTypes) + { + var sources = new HashSet(); + var queue = new Queue(); + + foreach (var (elementId, contract) in external) + { + if (!contract.StartsWith(NotifyPropertyChanged, StringComparison.Ordinal)) + { + continue; + } + + var type = ContainingType(graph.TryGetCodeElement(elementId)); + if (type is not null && sources.Add(type.Id)) + { + queue.Enqueue(type.Id); + } + } + + // Add returning false doubles as the visited check, so a diamond cannot enqueue a type twice. + while (queue.Count > 0) + { + if (!derivedTypes.TryGetValue(queue.Dequeue(), out var derived)) + { + continue; + } + + foreach (var type in derived.Where(type => sources.Add(type.Id))) + { + queue.Enqueue(type.Id); + } + } + + return sources; + } + + /// + /// The types a serializer drives: everything carrying one of the + /// . Unlike the binding sources this is not spread down the + /// inheritance edges - none of those attributes is inherited, a derived type has to carry its own. + /// + private static HashSet FindSerializableTypes(Graph.CodeGraph graph) + { + return graph.Nodes.Values + .Where(element => element.IsType() && element.Attributes.Any(SerializationAttributes.Contains)) + .Select(element => element.Id) + .ToHashSet(); + } + + private static CodeElement? ContainingType(CodeElement? element) + { + var current = element; + while (current is not null && !current.IsType()) + { + current = current.Parent; + } + + return current; + } + + /// Everything a relationship enters from the outside, plus what a used contract keeps alive. + private static HashSet ComputeReferenced( + List<(CodeElement Source, CodeElement Target)> referenceEdges, + Dictionary> implementations) + { + var referenced = new HashSet(); + + // Reused across relationships to keep the walk allocation free. + var sourceChain = new HashSet(); + + foreach (var (source, target) in referenceEdges) + { + MarkReferenced(source, target, referenced, sourceChain); + } + + PropagateContractUsage(referenced, implementations); + return referenced; + } + + private static void CollectEdges(Graph.CodeGraph graph, + List<(CodeElement Source, CodeElement Target)> referenceEdges, + Dictionary external, + Dictionary> implementations, Dictionary> contracts, + Dictionary> derivedTypes) + { + foreach (var relationship in graph.GetAllRelationships()) + { + var source = graph.TryGetCodeElement(relationship.SourceId); + var target = graph.TryGetCodeElement(relationship.TargetId); + if (source is null || target is null) + { + continue; + } + + if (relationship.Type == RelationshipType.Inherits && source.IsType() && target.IsType()) + { + Add(derivedTypes, target.Id, source); + } + + if (IsPolymorphicEdge(relationship.Type, source)) + { + RecordPolymorphicEdge(source, target, external, implementations, contracts); + continue; + } + + // Containment, Bundled and Handles are not references. Handles (handler -> event) is the + // callback wiring; the registration site itself produces the method group "Uses" edge that + // keeps the handler alive, so nothing is lost by ignoring it here. + if (!relationship.Type.IsDependency()) + { + continue; + } + + referenceEdges.Add((source, target)); + } + } + + /// + /// A relationship keeps alive every element whose subtree it enters from the outside: walking up + /// from the target, that is everything below the lowest common ancestor with the source. The common + /// ancestor and everything above it contain the source as well, so for them the relationship is an + /// internal one and proves nothing. + /// + private static void MarkReferenced(CodeElement source, CodeElement target, HashSet referenced, + HashSet sourceChain) + { + sourceChain.Clear(); + for (var current = source; current is not null; current = current.Parent) + { + sourceChain.Add(current.Id); + } + + for (var current = target; current is not null; current = current.Parent) + { + if (sourceChain.Contains(current.Id)) + { + break; + } + + referenced.Add(current.Id); + } + } + + /// + /// Marks an element alive whose caller is not part of the graph (a contract call reaching all + /// implementations). Without a source there is no common ancestor, so the whole chain is alive. + /// + private static void MarkReferencedFromOutside(CodeElement element, HashSet referenced) + { + for (var current = element; current is not null; current = current.Parent) + { + referenced.Add(current.Id); + } + } + + /// + /// "Implements" and "Overrides" starting at a member express polymorphism, not use. Starting at a + /// type ("class C : IFoo") the same relationship names the interface in C's declaration and is an + /// ordinary reference - which is why the source, not the target, decides. + /// + private static bool IsPolymorphicEdge(RelationshipType type, CodeElement source) + { + return (type is RelationshipType.Implements or RelationshipType.Overrides) && !source.IsType(); + } + + private static void RecordPolymorphicEdge(CodeElement source, CodeElement target, + Dictionary external, + Dictionary> implementations, Dictionary> contracts) + { + if (target.IsExternal || target.IsType()) + { + // Either a framework contract, or the parser's fallback to the containing type because it + // could not resolve the exact base member (generic base methods). Both mean the caller is + // invisible. Recorded on the member only - implementing IDisposable is not a use of the class, + // so the class itself stays reportable as dead code. + external.TryAdd(source.Id, target.FullName); + return; + } + + Add(implementations, target.Id, source); + Add(contracts, source.Id, target); + } + + /// + /// Pushes liveness from a used contract member to its implementations: calling IFoo.Bar calls every + /// implementation of Bar, so the implementations and the types holding them are alive. Transitive, + /// because an override can itself be overridden. + /// + private static void PropagateContractUsage(HashSet referenced, + Dictionary> implementations) + { + var queue = new Queue(implementations.Keys.Where(referenced.Contains)); + var enqueued = new HashSet(queue); + + while (queue.Count > 0) + { + foreach (var implementation in implementations[queue.Dequeue()]) + { + MarkReferencedFromOutside(implementation, referenced); + + if (implementations.ContainsKey(implementation.Id) && enqueued.Add(implementation.Id)) + { + queue.Enqueue(implementation.Id); + } + } + } + } + + /// + /// Everything unreferenced, reduced to the topmost element of each dead subtree. + /// + private static List Report(Graph.CodeGraph graph, HashSet referenced, + AnalysisContext context) + { + var findings = new List(); + + foreach (var element in graph.Nodes.Values) + { + // An external contract does not make the element alive - it is reported with a note instead, + // so the decision stays visible rather than silently removing rows from the result. + if (!IsCandidate(element) || referenced.Contains(element.Id)) + { + continue; + } + + // A single accessor is never a finding of its own: the question is whether the property is + // used, not whether both halves of it are. One unused half is the normal shape of anything a + // serializer, a binding or a framework drives - a DTO that is written in C# and only read by + // System.Text.Json has a dead getter on every single property. A property that is dead as a + // whole is still reported, as the property. + if (element.ElementType == CodeElementType.PropertyAccessor) + { + continue; + } + + if (IsSerializedProperty(element, context.SerializableTypes)) + { + continue; + } + + // Roll-up: a dead element inside a dead element is reported as part of it. Namespaces and + // assemblies are no candidates, so an element directly below them is always the topmost one. + var parent = element.Parent; + if (parent is not null && IsCandidate(parent) && !referenced.Contains(parent.Id)) + { + continue; + } + + findings.Add(CreateFinding(element, context)); + } + + return findings; + } + + private static DeadCodeFinding CreateFinding(CodeElement element, AnalysisContext context) + { + var hints = DeadCodeHint.None; + var attributes = new SortedSet(StringComparer.Ordinal); + + // The hints are collected over the whole subtree: what is reported is a dead class, but the + // evidence that it may still be alive usually sits on its members ([Test] methods, Main, ...). + foreach (var member in element.GetSubtreeIncludingSelf()) + { + if (IsEntryPoint(member)) + { + hints |= DeadCodeHint.EntryPoint; + } + + foreach (var attribute in member.Attributes) + { + if (TestAttributes.Contains(attribute)) + { + hints |= DeadCodeHint.TestCode; + } + else + { + hints |= DeadCodeHint.Attributed; + } + + attributes.Add(attribute); + } + } + + var related = new List(); + + // Reported although it implements an internal contract means the contract is dead as well - + // otherwise the propagation would have marked this element alive. + if (context.Contracts.TryGetValue(element.Id, out var implemented)) + { + hints |= DeadCodeHint.ImplementsDeadContract; + related.AddRange(implemented); + } + + if (context.Implementations.TryGetValue(element.Id, out var implementors)) + { + hints |= DeadCodeHint.ContractNeverCalled; + related.AddRange(implementors); + } + + // Element level only. A dead class whose members implement IDisposable is still dead - saying + // "might be used" about the class would be wrong, the note belongs to the member. + context.External.TryGetValue(element.Id, out var externalContract); + if (externalContract is not null) + { + hints |= DeadCodeHint.ImplementsExternalContract; + } + + return new DeadCodeFinding(element) + { + Confidence = RateConfidence(element, hints, context), + Hints = hints, + Attributes = attributes.ToList(), + RelatedMembers = related, + ExternalContract = externalContract + }; + } + + /// + /// Three rules, in order. A note about a caller outside the graph beats everything - we already + /// know the finding may be wrong. Otherwise visibility decides. + /// + private static DeadCodeConfidence RateConfidence(CodeElement element, DeadCodeHint hints, + AnalysisContext context) + { + if ((hints & CallerOutsideTheGraph) != DeadCodeHint.None) + { + return DeadCodeConfidence.Low; + } + + if (IsConfinedToAnalyzedCode(element) && !IsBindable(element, context.BindingSources)) + { + return DeadCodeConfidence.High; + } + + return DeadCodeConfidence.Medium; + } + + /// + /// Whether a XAML {Binding} could read this element without us seeing it. That takes two + /// things: a public property - the binding engine resolves by public reflection, so private, + /// internal and protected members are out of its reach - on a type that raises change + /// notifications. + /// + /// Note that being confined does not help here. A public property of an internal class cannot + /// be referenced from another assembly, but the binding sits inside the assembly and is + /// merely invisible, which is a different thing. + /// + /// + private static bool IsBindable(CodeElement element, HashSet bindingSources) + { + return IsPublicPropertyOf(element, bindingSources); + } + + /// + /// Whether the element is a public property of a type marked as a serialization target. The + /// serializer reaches it by reflection, so "nothing references it" says nothing about it at all - + /// such a property is not reported. + /// + /// Dropping it rather than reporting it with a note is deliberate: on a DTO every + /// property looks dead, so the note would be the rule rather than the exception and would fill + /// the result with rows nobody can act on. + /// + /// + private static bool IsSerializedProperty(CodeElement element, HashSet serializableTypes) + { + return IsPublicPropertyOf(element, serializableTypes); + } + + /// + /// Whether the element is a public property of one of the given types. Accessors are never findings + /// of their own (see ), so the reported element is the property itself. + /// + private static bool IsPublicPropertyOf(CodeElement element, HashSet types) + { + if (element is not { ElementType: CodeElementType.Property, AccessLevel: AccessLevel.Public }) + { + return false; + } + + var type = ContainingType(element); + return type is not null && types.Contains(type.Id); + } + + /// + /// Whether the element is out of reach for code we did not analyze. It is enough that *any* + /// container is private or internal: a public method of an internal class cannot be called from + /// another assembly either. An element whose visibility is unknown contributes nothing, so a graph + /// from an importer that does not supply it never reaches high confidence. + /// + /// "InternalsVisibleTo" is not considered. A friend assembly inside the analysis would show its + /// references anyway; one outside it is the rare case this misses. + /// + /// + private static bool IsConfinedToAnalyzedCode(CodeElement element) + { + for (var current = element; current is not null; current = current.Parent) + { + if (current.AccessLevel.IsConfinedToAnalyzedCode()) + { + return true; + } + } + + return false; + } + + /// + /// Containers never carry relationships of their own, so they would all look dead. External + /// elements are out of scope - we see neither their callers nor their bodies. + /// + private static bool IsCandidate(CodeElement element) + { + return !element.IsExternal && + element.ElementType is not (CodeElementType.Assembly or CodeElementType.Namespace); + } + + /// + /// Started from outside the analyzed code: the program entry point, and the synthetic + /// "GlobalStatements" class the parser creates per assembly for top-level statements. + /// + private static bool IsEntryPoint(CodeElement element) + { + if (element is { ElementType: CodeElementType.Method, Name: "Main" }) + { + return true; + } + + // A static constructor is run by the runtime before the first use of the type. Nothing in the + // code ever references it, so without this it looks like a particularly trustworthy finding - + // it is usually private, which would otherwise put it in the highest confidence band. + if (element is { ElementType: CodeElementType.Method, Name: ".cctor" }) + { + return true; + } + + return element is { ElementType: CodeElementType.Class, Name: "GlobalStatements" } && + (element.Parent?.ElementType == CodeElementType.Assembly || + element.Parent is { ElementType: CodeElementType.Namespace, Name: CodeElement.GlobalNamespaceName }); + } + + private static void Add(Dictionary> map, string key, CodeElement value) + { + if (!map.TryGetValue(key, out var list)) + { + list = []; + map[key] = list; + } + + list.Add(value); + } +} diff --git a/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeFinding.cs b/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeFinding.cs new file mode 100644 index 00000000..935971cf --- /dev/null +++ b/CSharpCodeAnalyst.CodeGraph/Algorithms/DeadCode/DeadCodeFinding.cs @@ -0,0 +1,99 @@ +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; + +/// +/// Reasons why a reported element may nevertheless be alive. The analysis works on the relationships +/// the parser could see, so everything reached through XAML, reflection, dependency injection or +/// serialization looks unreferenced. Rather than silently dropping such elements, they are reported +/// with the hint that explains why they are suspicious - the caller decides what to do with them. +/// +[Flags] +public enum DeadCodeHint +{ + None = 0, + + /// Called from outside the analyzed code by definition (program entry point). + EntryPoint = 1, + + /// The element or something in its subtree carries a known test-framework attribute. + TestCode = 2, + + /// Carries attributes. Attributes often mean an external framework drives the element. + Attributed = 4, + + /// + /// A contract member (interface or base member) that is implemented but never called through the + /// contract - the abstraction itself is unused. + /// + ContractNeverCalled = 8, + + /// + /// Implements or overrides an internal contract member that is itself dead, so it can only be + /// removed together with that contract. + /// + ImplementsDeadContract = 16, + + /// + /// Implements or overrides a contract from outside the analyzed code (a framework interface, a + /// base member from a referenced assembly). The caller is the framework, so nothing in the graph + /// references it - it is almost certainly alive. + /// + ImplementsExternalContract = 32 +} + +/// +/// How much the finding can be trusted. Three levels, each from one stated rule - this is a summary of +/// what we know, not a measurement. +/// +public enum DeadCodeConfidence +{ + /// + /// A note says the caller may sit outside the graph (entry point, test code, attributes, an + /// external contract). We know we might be wrong here. + /// + Low, + + /// + /// Nothing references it, but it could be reached from code we did not analyze - it is public or + /// protected, or the producer did not tell us its visibility. + /// + Medium, + + /// + /// Nothing references it, and nothing outside the analyzed code could: the element or one of its + /// containers is private or internal. "Nothing references it" and "nothing can reference it" mean + /// the same thing here. + /// + High +} + +/// +/// One reported element: the topmost element of a dead subtree, plus what we know about it. +/// +public sealed class DeadCodeFinding(CodeElement element) +{ + /// The unreferenced element. Everything below it is dead too and is not reported separately. + public CodeElement Element { get; } = element; + + /// How much the finding can be trusted - see . + public DeadCodeConfidence Confidence { get; init; } = DeadCodeConfidence.Medium; + + public DeadCodeHint Hints { get; init; } + + /// Distinct attribute names found on the element and its subtree. + public IReadOnlyList Attributes { get; init; } = []; + + /// + /// The contract outside the analyzed code this element implements or overrides, e.g. + /// "IDisposable.Dispose". Set together with . + /// + public string? ExternalContract { get; init; } + + /// + /// The polymorphically related members: the internal contract members this element implements + /// () and the implementations that die with it + /// (). + /// + public IReadOnlyList RelatedMembers { get; init; } = []; +} diff --git a/CSharpCodeAnalyst.CodeGraph/Contracts/ParseResult.cs b/CSharpCodeAnalyst.CodeGraph/Contracts/ParseResult.cs index f64d6079..84ba77e8 100644 --- a/CSharpCodeAnalyst.CodeGraph/Contracts/ParseResult.cs +++ b/CSharpCodeAnalyst.CodeGraph/Contracts/ParseResult.cs @@ -1,13 +1,21 @@ +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Metrics; namespace CSharpCodeAnalyst.CodeGraph.Contracts; /// -/// The complete output of a parse or import: the code graph together with the (optional) -/// per-member source metrics collected alongside it. Bundling them makes it explicit that both -/// belong to the same run and travel together - there is no separate, mutable "last metrics" -/// state on the producer. +/// The complete output of a parse or import: the code graph together with the (optional) per-member +/// facts collected alongside it - source metrics, and the contracts implemented from outside the +/// analyzed code. Bundling them makes it explicit that they belong to the same run and travel +/// together - there is no separate, mutable "last metrics" state on the producer. /// Lives here rather than next to the C# parser because every graph producer returns one, and /// the importers must not have to reference the Roslyn-based parser to do so. +/// +/// is optional and defaults to an empty store, so a producer that +/// knows nothing about it (every importer) stays unchanged. +/// /// -public sealed record ParseResult(Graph.CodeGraph CodeGraph, MetricStore Metrics); +public sealed record ParseResult(Graph.CodeGraph CodeGraph, MetricStore Metrics) +{ + public ExternalContractStore ExternalContracts { get; init; } = new(); +} diff --git a/CSharpCodeAnalyst.CodeGraph/Declarations/ExternalContractStore.cs b/CSharpCodeAnalyst.CodeGraph/Declarations/ExternalContractStore.cs new file mode 100644 index 00000000..825eab78 --- /dev/null +++ b/CSharpCodeAnalyst.CodeGraph/Declarations/ExternalContractStore.cs @@ -0,0 +1,74 @@ +using System.Collections.Concurrent; + +namespace CSharpCodeAnalyst.CodeGraph.Declarations; + +/// +/// Records which members implement or override a contract that is not part of the analyzed +/// code - a framework interface member (ICommand.Execute) or a base member from a +/// referenced assembly (object.ToString, CSharpSyntaxVisitor.VisitGenericName), keyed by +/// and valued with the contract's display name. +/// +/// Such a member has no incoming reference anywhere in the graph - the caller is the framework - +/// so without this information it looks like dead code. The relationship model cannot carry the +/// fact: with external code excluded there is no element to point an Overrides edge at, and +/// with it included the edge is flattened to a Uses edge on the containing type, which is +/// indistinguishable from ordinary use of that type. +/// +/// +/// Kept beside the code graph rather than on , following +/// : the graph model stays pure, the store is trivially optional +/// (an importer that knows nothing about this simply leaves it empty), and the shared +/// type does not grow a field that only the C# parser ever fills. +/// +/// +/// Filled from the parallel phase 2 of the parser, hence the concurrent dictionary. +/// +/// +public sealed class ExternalContractStore +{ + private readonly ConcurrentDictionary _contracts = new(); + + public IReadOnlyDictionary Contracts => _contracts; + + public int Count => _contracts.Count; + + public bool IsEmpty => _contracts.IsEmpty; + + /// + /// Records the contract for an element. A member can implement several external contracts; the + /// first one wins, because the store answers "is this member bound by code we cannot see" and one + /// example is enough to explain it. + /// + public void Add(string elementId, string contractName) + { + _contracts.TryAdd(elementId, contractName); + } + + public string? TryGet(string elementId) + { + return _contracts.GetValueOrDefault(elementId); + } + + public bool Contains(string elementId) + { + return _contracts.ContainsKey(elementId); + } + + public void Clear() + { + _contracts.Clear(); + } + + /// + /// Replaces the current contents. Used to refill the shared store after an import or when loading + /// a project. + /// + public void LoadFrom(IReadOnlyDictionary contracts) + { + _contracts.Clear(); + foreach (var (id, contract) in contracts) + { + _contracts[id] = contract; + } + } +} diff --git a/CSharpCodeAnalyst.CodeGraph/Export/CodeGraphSerializer.cs b/CSharpCodeAnalyst.CodeGraph/Export/CodeGraphSerializer.cs index 30053320..64a2c5d6 100644 --- a/CSharpCodeAnalyst.CodeGraph/Export/CodeGraphSerializer.cs +++ b/CSharpCodeAnalyst.CodeGraph/Export/CodeGraphSerializer.cs @@ -76,6 +76,11 @@ private static void SerializeElement(StringBuilder sb, CodeElement element) sb.Append($"{Separator}external"); } + if (element.AccessLevel != AccessLevel.Unknown) + { + sb.Append($"{Separator}access={element.AccessLevel}"); + } + if (element.Attributes.Count > 0) { var attrs = string.Join(",", element.Attributes.OrderBy(a => a)); @@ -256,6 +261,7 @@ private static (CodeElement element, string? parentId, int linesConsumed) ParseE string? fullName = null; string? parentId = null; var isExternal = false; + var accessLevel = AccessLevel.Unknown; var attributes = new HashSet(); // Parse optional fields @@ -279,6 +285,11 @@ private static (CodeElement element, string? parentId, int linesConsumed) ParseE { isExternal = true; } + else if (part.StartsWith("access=")) + { + // An unreadable value stays Unknown - never guess a visibility. + Enum.TryParse(part.Substring("access=".Length), out accessLevel); + } else if (part.StartsWith("attr=")) { var attrList = part.Substring("attr=".Length).Split(','); @@ -296,7 +307,8 @@ private static (CodeElement element, string? parentId, int linesConsumed) ParseE // Create element without parent - will be linked later var element = new CodeElement(id, elementType, name, fullName, null) { - IsExternal = isExternal + IsExternal = isExternal, + AccessLevel = accessLevel }; foreach (var attr in attributes) diff --git a/CSharpCodeAnalyst.CodeGraph/Graph/AccessLevel.cs b/CSharpCodeAnalyst.CodeGraph/Graph/AccessLevel.cs new file mode 100644 index 00000000..7f50e1ea --- /dev/null +++ b/CSharpCodeAnalyst.CodeGraph/Graph/AccessLevel.cs @@ -0,0 +1,56 @@ +namespace CSharpCodeAnalyst.CodeGraph.Graph; + +/// +/// How far a code element can be reached from. Modelled after C#, but the concept exists in every +/// language the tool imports. +/// +/// is the default and means exactly that: nobody told us. Every importer +/// that does not know about visibility leaves it there, and so does a project file written before +/// this existed. It must never be read as "public" or as "private" - an analysis that draws a +/// conclusion from visibility has to treat Unknown as "no information". +/// +/// +/// Deliberately not called "Accessibility": WPF drags a global Accessibility namespace into +/// scope, so every file in the UI projects would have to fully qualify the type. +/// +/// +public enum AccessLevel +{ + Unknown, + + /// Reachable only from inside the declaring type. + Private, + + /// Reachable from the declaring type and everything derived from it. + Protected, + + /// Reachable from inside the declaring assembly. + Internal, + + /// C# "private protected": derived types, but only within the declaring assembly. + ProtectedAndInternal, + + /// C# "protected internal": the declaring assembly, plus derived types anywhere. + ProtectedOrInternal, + + /// Reachable from anywhere, including code that is not part of the analysis. + Public +} + +public static class AccessLevelExtensions +{ + /// + /// Whether everything that could reach this element is necessarily part of the analyzed code. Only + /// then is "nothing references it" the same as "nothing can reference it". + /// + /// Private and internal (in either combination) are confined to the declaring type or assembly, + /// both of which we analyzed. Protected and public can be reached from code outside the + /// analysis, and tells us nothing at all - all three answer + /// false. + /// + /// + public static bool IsConfinedToAnalyzedCode(this AccessLevel accessLevel) + { + return accessLevel is AccessLevel.Private or AccessLevel.Internal or AccessLevel.ProtectedAndInternal; + } +} diff --git a/CSharpCodeAnalyst.CodeGraph/Graph/CodeElement.cs b/CSharpCodeAnalyst.CodeGraph/Graph/CodeElement.cs index de370251..9a27af5c 100644 --- a/CSharpCodeAnalyst.CodeGraph/Graph/CodeElement.cs +++ b/CSharpCodeAnalyst.CodeGraph/Graph/CodeElement.cs @@ -41,6 +41,13 @@ public class CodeElement(string id, CodeElementType elementType, string name, st /// public bool IsExternal { get; init; } + /// + /// How far the element can be reached from. when the + /// producer does not supply it - every importer except the C# parser, and any project file written + /// before this existed. Never read Unknown as a value; it means "no information". + /// + public AccessLevel AccessLevel { get; init; } + public override bool Equals(object? obj) { if (obj != null && obj.GetType() == GetType()) @@ -107,7 +114,8 @@ public CodeElement CloneSimple() var element = new CodeElement(Id, ElementType, Name, FullName, null) { - IsExternal = IsExternal + IsExternal = IsExternal, + AccessLevel = AccessLevel }; element.SourceLocations.AddRange(SourceLocations); diff --git a/CSharpCodeAnalyst.CodeGraph/Graph/RelationshipAttribute.cs b/CSharpCodeAnalyst.CodeGraph/Graph/RelationshipAttribute.cs index e3c77592..4e0c500f 100644 --- a/CSharpCodeAnalyst.CodeGraph/Graph/RelationshipAttribute.cs +++ b/CSharpCodeAnalyst.CodeGraph/Graph/RelationshipAttribute.cs @@ -17,7 +17,14 @@ public enum RelationshipAttribute : uint IsExtensionMethodCall = 16, IsMethodGroup = 32, EventRegistration = 64, - EventUnregistration = 128 + EventUnregistration = 128, + + /// + /// The relationship was read out of a XAML file, not out of C#. Declarative XAML (element tags, + /// {x:Static}, {x:Type}) is compiled into BAML and resolved by reflection at runtime, so the + /// markup compiler generates no C# for it and Roslyn cannot see it. + /// + IsXamlReference = 256 } public static class RelationshipAttributeExtensions diff --git a/CSharpCodeAnalyst.CodeParser/Parser/Config/ParserConfig.cs b/CSharpCodeAnalyst.CodeParser/Parser/Config/ParserConfig.cs index 6cb505f5..647e301f 100644 --- a/CSharpCodeAnalyst.CodeParser/Parser/Config/ParserConfig.cs +++ b/CSharpCodeAnalyst.CodeParser/Parser/Config/ParserConfig.cs @@ -5,12 +5,14 @@ public class ParserConfig private readonly ProjectExclusionRegExCollection _projectExclusionFilters; public ParserConfig(ProjectExclusionRegExCollection projectExclusionFilters, bool includeExternals, - bool includeGeneratedCode = false, bool splitPropertyAccessors = false) + bool includeGeneratedCode = false, bool splitPropertyAccessors = false, + bool includeXamlReferences = true) { _projectExclusionFilters = projectExclusionFilters; IncludeExternals = includeExternals; IncludeGeneratedCode = includeGeneratedCode; SplitPropertyAccessors = splitPropertyAccessors; + IncludeXamlReferences = includeXamlReferences; } public bool IncludeExternals { get; } @@ -31,6 +33,14 @@ public ParserConfig(ProjectExclusionRegExCollection projectExclusionFilters, boo /// public bool SplitPropertyAccessors { get; } + /// + /// When enabled, the XAML files next to the analyzed projects are scanned for the references the + /// markup compiler does not turn into C# (element tags, {x:Static}, {x:Type}) and + /// those become relationships in the graph. Without it a control that is only instantiated from + /// XAML looks unreferenced. + /// + public bool IncludeXamlReferences { get; } + public bool IsProjectIncluded(string projectName) { diff --git a/CSharpCodeAnalyst.CodeParser/Parser/DeclarationAnalyzer.cs b/CSharpCodeAnalyst.CodeParser/Parser/DeclarationAnalyzer.cs index c14249bd..8ad53dcf 100644 --- a/CSharpCodeAnalyst.CodeParser/Parser/DeclarationAnalyzer.cs +++ b/CSharpCodeAnalyst.CodeParser/Parser/DeclarationAnalyzer.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Graph; using CSharpCodeAnalyst.CodeParser.Parser.Config; using Microsoft.CodeAnalysis; @@ -20,14 +21,16 @@ internal class DeclarationAnalyzer private readonly SyntaxNodeAnalyzer _bodyAnalyzer; private readonly RelationshipBuilder _builder; private readonly ParserConfig _config; + private readonly ExternalContractStore _externalContracts; internal DeclarationAnalyzer(RelationshipBuilder builder, SyntaxNodeAnalyzer bodyAnalyzer, Artifacts artifacts, - ParserConfig config) + ParserConfig config, ExternalContractStore externalContracts) { _builder = builder; _bodyAnalyzer = bodyAnalyzer; _artifacts = artifacts; _config = config; + _externalContracts = externalContracts; } /// @@ -50,6 +53,7 @@ public void Analyze(Solution solution, CodeElement element, ISymbol symbol) AnalyzeInheritanceRelationships(element, typeSymbol); AnalyzeEnumMemberInitializers(solution, element, typeSymbol); AnalyzePrimaryConstructorBaseArguments(solution, element, typeSymbol); + RecordExternalInterfaceImplementations(typeSymbol); } else if (symbol is IMethodSymbol methodSymbol) { @@ -390,6 +394,69 @@ private void AddMethodOverrideRelationship(CodeElement sourceElement, IMethodSym // Maybe we override a framework method. Happens also if the base method is a generic one. // In this case the GetSymbolKey is different. One uses T, the overriding method uses the actual type. _builder.AddRelationshipWithFallbackToContainingType(sourceElement, methodSymbol, RelationshipType.Overrides, locations, RelationshipAttribute.None); + + RecordIfExternalContract(sourceElement, methodSymbol); + } + + /// + /// An override whose base member lives outside the analyzed code produces no relationship at all - + /// there is no element to point at - so the member ends up without a single incoming reference and + /// looks like dead code. The fact is recorded beside the graph instead. + /// The containing type decides: when it is one of ours the contract is internal, and the + /// edge already expresses it. + /// + private void RecordIfExternalContract(CodeElement sourceElement, ISymbol contractMember) + { + var containingType = contractMember.ContainingType; + if (containingType is null || _builder.FindInternalCodeElement(containingType.OriginalDefinition) is not null) + { + return; + } + + _externalContracts.Add(sourceElement.Id, $"{containingType.Name}.{contractMember.Name}"); + } + + /// + /// Members that implement an interface from outside the analyzed code (ICommand.Execute, + /// IValueConverter.Convert, ...). Nothing in the graph shows this: the interface is not an + /// element, and the implementation is called by the framework, never from our code. + /// + /// Only foreign interfaces are scanned. For our own, + /// creates real edges from the interface side. + /// + /// + /// The interfaces come from and are therefore + /// already constructed, so + /// can be called directly - the mapping trap described at + /// does not apply here. + /// + /// + private void RecordExternalInterfaceImplementations(INamedTypeSymbol typeSymbol) + { + foreach (var contract in typeSymbol.AllInterfaces) + { + // A constructed generic interface (IHandler) is not in the map - the definition is. + if (_builder.FindInternalCodeElement(contract.OriginalDefinition) is not null) + { + continue; + } + + foreach (var contractMember in contract.GetMembers()) + { + var implementation = typeSymbol.FindImplementationForInterfaceMember(contractMember); + if (implementation is null) + { + continue; + } + + var element = _builder.FindInternalCodeElement(implementation) + ?? _builder.FindInternalCodeElement(implementation.OriginalDefinition); + if (element is not null) + { + _externalContracts.Add(element.Id, $"{contract.Name}.{contractMember.Name}"); + } + } + } } private void AnalyzeFieldRelationships(Solution solution, CodeElement fieldElement, IFieldSymbol fieldSymbol) @@ -553,6 +620,8 @@ private void AnalyzePropertyAbstractions(CodeElement propertyElement, IPropertyS { _builder.AddRelationshipWithFallbackToContainingType(propertyElement, overriddenProperty, RelationshipType.Overrides, propertySymbol.GetSymbolLocations(), RelationshipAttribute.None); + + RecordIfExternalContract(propertyElement, overriddenProperty); } } diff --git a/CSharpCodeAnalyst.CodeParser/Parser/HierarchyAnalyzer.cs b/CSharpCodeAnalyst.CodeParser/Parser/HierarchyAnalyzer.cs index cd9c6d69..173d65cc 100644 --- a/CSharpCodeAnalyst.CodeParser/Parser/HierarchyAnalyzer.cs +++ b/CSharpCodeAnalyst.CodeParser/Parser/HierarchyAnalyzer.cs @@ -176,6 +176,27 @@ private bool ShouldAnalyzeProject(Project project) return true; } + /// + /// Roslyn's accessibility onto ours. NotApplicable (namespaces, and anything Roslyn cannot + /// decide) maps to Unknown - the graph must not claim a visibility that does not exist. + /// + private static CodeGraph.Graph.AccessLevel MapAccessLevel( + Microsoft.CodeAnalysis.Accessibility accessibility) + { + return accessibility switch + { + Microsoft.CodeAnalysis.Accessibility.Private => CodeGraph.Graph.AccessLevel.Private, + Microsoft.CodeAnalysis.Accessibility.Protected => CodeGraph.Graph.AccessLevel.Protected, + Microsoft.CodeAnalysis.Accessibility.Internal => CodeGraph.Graph.AccessLevel.Internal, + Microsoft.CodeAnalysis.Accessibility.ProtectedAndInternal => CodeGraph.Graph.AccessLevel + .ProtectedAndInternal, + Microsoft.CodeAnalysis.Accessibility.ProtectedOrInternal => CodeGraph.Graph.AccessLevel + .ProtectedOrInternal, + Microsoft.CodeAnalysis.Accessibility.Public => CodeGraph.Graph.AccessLevel.Public, + _ => CodeGraph.Graph.AccessLevel.Unknown + }; + } + private async Task BuildHierarchy(Compilation compilation, IEnumerable generatedDocuments) { // Assembly has no source location. @@ -429,7 +450,10 @@ private CodeElement GetOrCreateCodeElement(ISymbol symbol, CodeElementType eleme var fullName = symbol.BuildSymbolName(); var newId = Guid.NewGuid().ToString(); - var element = new CodeElement(newId, elementType, name, fullName, parent); + var element = new CodeElement(newId, elementType, name, fullName, parent) + { + AccessLevel = MapAccessLevel(symbol.DeclaredAccessibility) + }; UpdateCodeElementLocations(element, location); @@ -478,7 +502,11 @@ private void CreatePropertyAccessorElement(IMethodSymbol? accessor, CodeElement var name = accessor.Name; var fullName = propertyElement.FullName + "." + name; var id = Guid.NewGuid().ToString(); - var accessorElement = new CodeElement(id, CodeElementType.PropertyAccessor, name, fullName, propertyElement); + var accessorElement = new CodeElement(id, CodeElementType.PropertyAccessor, name, fullName, propertyElement) + { + // An accessor may narrow the property ("public int P { get; private set; }"). + AccessLevel = MapAccessLevel(accessor.DeclaredAccessibility) + }; foreach (var accessorLocation in accessor.GetSymbolLocations()) { diff --git a/CSharpCodeAnalyst.CodeParser/Parser/Parser.cs b/CSharpCodeAnalyst.CodeParser/Parser/Parser.cs index 54a31abd..b3cf8447 100644 --- a/CSharpCodeAnalyst.CodeParser/Parser/Parser.cs +++ b/CSharpCodeAnalyst.CodeParser/Parser/Parser.cs @@ -1,8 +1,10 @@ using System.Diagnostics; using CSharpCodeAnalyst.CodeGraph.Contracts; +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Graph; using CSharpCodeAnalyst.CodeGraph.Metrics; using CSharpCodeAnalyst.CodeParser.Parser.Config; +using CSharpCodeAnalyst.CodeParser.Xaml; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.MSBuild; @@ -195,12 +197,20 @@ private async Task ParseSolutionInternal(Solution solution) sw = Stopwatch.StartNew(); // Second Pass: Build Relationships + var externalContracts = new ExternalContractStore(); var phase2 = new RelationshipAnalyzer(progress, config); - await phase2.AnalyzeRelationships(solution, codeGraph, artifacts); + await phase2.AnalyzeRelationships(solution, codeGraph, artifacts, externalContracts); sw.Stop(); Trace.TraceInformation("Analyzing relationships: " + sw.Elapsed); + // Third pass: the XAML references Roslyn cannot see. Runs before the global namespace is inserted + // so the synthetic elements for code-behind-less files are moved along with everything else. + if (config.IncludeXamlReferences) + { + LinkXamlReferences(solution, codeGraph); + } + // Makes the cycle detection easier because I never get to the assembly as shared ancestor // for a nested relationships. InsertGlobalNamespaceIfUsed(codeGraph); @@ -210,10 +220,42 @@ private async Task ParseSolutionInternal(Solution solution) #endif //await File.WriteAllTextAsync("d:\\debug0.txt", codeGraph.ToDebug()); - return new ParseResult(codeGraph, metrics); + return new ParseResult(codeGraph, metrics) { ExternalContracts = externalContracts }; } + /// + /// Adds the references that only exist in XAML. The assembly elements are matched to the Roslyn + /// projects by assembly name, and each project contributes the XAML files below its own directory - + /// MSBuildWorkspace does not expose the "Page" items, so the directory is the practical source. + /// + private void LinkXamlReferences(Solution solution, CodeGraph.Graph.CodeGraph codeGraph) + { + progress?.Report("Reading XAML references ..."); + + var assembliesByName = codeGraph.GetRoots() + .Where(root => root.ElementType == CodeElementType.Assembly) + .ToDictionary(root => root.Name, root => root); + + var projects = new List(); + foreach (var project in solution.Projects) + { + var directory = Path.GetDirectoryName(project.FilePath); + if (directory is null || !config.IsProjectIncluded(project.Name) || + !assembliesByName.TryGetValue(project.AssemblyName, out var assembly)) + { + continue; + } + + projects.Add(new XamlProject(assembly, directory)); + } + + var sw = Stopwatch.StartNew(); + var added = XamlGraphLinker.Link(codeGraph, projects); + sw.Stop(); + Trace.TraceInformation($"Reading XAML references: {sw.Elapsed} ({added} relationships)"); + } + /// /// Computes per-member source metrics from the symbol map built in phase 1. /// Only method-like symbols with an actual implementation are measured; abstract/extern/ diff --git a/CSharpCodeAnalyst.CodeParser/Parser/RelationshipAnalyzer.cs b/CSharpCodeAnalyst.CodeParser/Parser/RelationshipAnalyzer.cs index 84140cf8..8af92a2f 100644 --- a/CSharpCodeAnalyst.CodeParser/Parser/RelationshipAnalyzer.cs +++ b/CSharpCodeAnalyst.CodeParser/Parser/RelationshipAnalyzer.cs @@ -1,3 +1,4 @@ +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Graph; using CSharpCodeAnalyst.CodeParser.Parser.Config; using Microsoft.CodeAnalysis; @@ -34,15 +35,16 @@ public RelationshipAnalyzer(IProgress? progress, ParserConfig config) /// (useful when debugging); the default (-1) lets the scheduler use all available cores. /// public Task AnalyzeRelationships(Solution solution, CodeGraph.Graph.CodeGraph codeGraph, Artifacts artifacts, - int maxDegreeOfParallelism = -1) + ExternalContractStore externalContracts, int maxDegreeOfParallelism = -1) { ArgumentNullException.ThrowIfNull(solution, nameof(solution)); ArgumentNullException.ThrowIfNull(codeGraph, nameof(codeGraph)); ArgumentNullException.ThrowIfNull(artifacts, nameof(artifacts)); + ArgumentNullException.ThrowIfNull(externalContracts, nameof(externalContracts)); var builder = new RelationshipBuilder(codeGraph, artifacts, _config); var bodyAnalyzer = new SyntaxNodeAnalyzer(builder, _config); - var declarationAnalyzer = new DeclarationAnalyzer(builder, bodyAnalyzer, artifacts, _config); + var declarationAnalyzer = new DeclarationAnalyzer(builder, bodyAnalyzer, artifacts, _config, externalContracts); var numberOfCodeElements = codeGraph.Nodes.Count; _processedCodeElements = 0; diff --git a/CSharpCodeAnalyst.CodeParser/Xaml/XamlGraphLinker.cs b/CSharpCodeAnalyst.CodeParser/Xaml/XamlGraphLinker.cs new file mode 100644 index 00000000..d000b29f --- /dev/null +++ b/CSharpCodeAnalyst.CodeParser/Xaml/XamlGraphLinker.cs @@ -0,0 +1,269 @@ +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CSharpCodeAnalyst.CodeParser.Xaml; + +/// One analyzed project: its assembly element in the graph and the directory to scan for XAML. +public sealed record XamlProject(CodeElement Assembly, string Directory); + +/// +/// Turns the references finds into real relationships in the code +/// graph, so a type that is only ever instantiated from XAML no longer looks unused. +/// +/// The source of such a relationship is the code-behind class named by x:Class. A resource +/// dictionary has no code-behind, so a synthetic class named after the file takes its place - +/// the same device the parser already uses for top-level statements ("GlobalStatements"). Those +/// synthetic elements have no incoming references of their own (nothing resolves the +/// MergedDictionaries URIs), so they show up in a dead code analysis. That is a known and +/// accepted cost; there were 14 of them in this repository against 1050 findings. +/// +/// +/// Resolution is by exact name, never by guessing: the xmlns gives the CLR namespace and optionally +/// the assembly. Without ;assembly= XAML means the assembly the file is compiled into, which +/// is what is tried first; a unique match elsewhere is accepted as a fallback. +/// +/// +public static class XamlGraphLinker +{ + /// The element name the parser gives a constructor (it comes straight from the symbol). + private const string ConstructorName = ".ctor"; + + public static int Link(CodeGraph.Graph.CodeGraph graph, IReadOnlyList projects) + { + ArgumentNullException.ThrowIfNull(graph); + ArgumentNullException.ThrowIfNull(projects); + + var typesByAssembly = BuildTypeLookup(graph); + var added = 0; + + foreach (var project in projects) + { + foreach (var file in EnumerateXamlFiles(project.Directory)) + { + added += LinkFile(graph, project, file, typesByAssembly); + } + } + + return added; + } + + private static int LinkFile(CodeGraph.Graph.CodeGraph graph, XamlProject project, string file, + Dictionary> typesByAssembly) + { + XamlFileReferences references; + try + { + references = XamlReferenceExtractor.Extract(File.ReadAllText(file)); + } + catch (IOException) + { + // An unreadable file must not break the parse run. + return 0; + } + + if (references.References.Count == 0) + { + return 0; + } + + var source = ResolveSource(graph, project, file, references, typesByAssembly); + var added = 0; + + foreach (var reference in references.References) + { + foreach (var target in ResolveTargets(project, reference, typesByAssembly)) + { + if (target.Id == source.Id) + { + continue; + } + + if (AddReference(source, target, file, reference)) + { + added++; + } + } + } + + return added; + } + + /// + /// Adds the relationship, or merges the location into the existing one. The relationship set is + /// keyed by (source, target, type), so a plain Add would silently drop the new source location. + /// + private static bool AddReference(CodeElement source, CodeElement target, string file, XamlReference reference) + { + var location = new SourceLocation(file, reference.Line, reference.Column); + var existing = source.Relationships.FirstOrDefault( + r => r.TargetId == target.Id && r.Type == RelationshipType.Uses); + + if (existing is not null) + { + if (!existing.SourceLocations.Contains(location)) + { + existing.SourceLocations.Add(location); + } + + existing.SetAttribute(RelationshipAttribute.IsXamlReference); + return false; + } + + var relationship = new Relationship(source.Id, target.Id, RelationshipType.Uses, + RelationshipAttribute.IsXamlReference); + relationship.SourceLocations.Add(location); + source.Relationships.Add(relationship); + return true; + } + + private static CodeElement ResolveSource(CodeGraph.Graph.CodeGraph graph, XamlProject project, string file, + XamlFileReferences references, Dictionary> typesByAssembly) + { + if (references.CodeBehindClass is not null && + typesByAssembly.TryGetValue(project.Assembly.Name, out var types) && + types.TryGetValue(references.CodeBehindClass, out var codeBehind)) + { + return codeBehind; + } + + return GetOrCreateSyntheticElement(graph, project, file); + } + + /// + /// The stand-in for a XAML file that has no code-behind class. Named after the path relative to the + /// project so two files with the same name stay distinguishable. + /// + private static CodeElement GetOrCreateSyntheticElement(CodeGraph.Graph.CodeGraph graph, XamlProject project, + string file) + { + var name = Path.ChangeExtension(Path.GetRelativePath(project.Directory, file), null) + .Replace(Path.DirectorySeparatorChar, '.') + .Replace(Path.AltDirectorySeparatorChar, '.'); + + var fullName = project.Assembly.FullName + "." + name; + + var existing = project.Assembly.Children.FirstOrDefault(c => c.FullName == fullName); + if (existing is not null) + { + return existing; + } + + var element = new CodeElement(Guid.NewGuid().ToString(), CodeElementType.Class, name, fullName, + project.Assembly); + element.SourceLocations.Add(new SourceLocation(file, 1, 1)); + + project.Assembly.Children.Add(element); + graph.Nodes[element.Id] = element; + return element; + } + + private static IEnumerable ResolveTargets(XamlProject project, XamlReference reference, + Dictionary> typesByAssembly) + { + var type = ResolveType(project, reference, typesByAssembly); + if (type is null) + { + yield break; + } + + if (reference.MemberName is not null) + { + // {x:Static Type.Member} - prefer the member, fall back to the type when it has no element + // (e.g. an enum value or a member the parser did not model). + yield return type.Children.FirstOrDefault(c => c.Name == reference.MemberName) ?? type; + yield break; + } + + yield return type; + + if (!reference.IsInstantiation) + { + yield break; + } + + // An object element runs the constructor. Without this edge the constructor has no incoming + // reference at all, and everything only it calls dies with it in the cascade - the body of a + // XAML-instantiated control lives almost entirely below its constructor. + // Overloads share the element name, so all of them are linked: XAML picks the parameterless one, + // but the graph cannot tell them apart, and an edge too many is far cheaper here than a missing + // one. + foreach (var constructor in type.Children.Where(IsConstructor)) + { + yield return constructor; + } + } + + private static bool IsConstructor(CodeElement element) + { + return element is { ElementType: CodeElementType.Method, Name: ConstructorName }; + } + + private static CodeElement? ResolveType(XamlProject project, XamlReference reference, + Dictionary> typesByAssembly) + { + // An explicit ";assembly=" wins; without it XAML means the assembly the file is compiled into. + var assemblyName = reference.AssemblyName ?? project.Assembly.Name; + if (typesByAssembly.TryGetValue(assemblyName, out var types) && + types.TryGetValue(reference.TypeFullName, out var declared)) + { + return declared; + } + + // Fallback: a unique match anywhere. Ambiguous names are dropped rather than guessed. + var matches = typesByAssembly.Values + .Select(candidates => candidates.GetValueOrDefault(reference.TypeFullName)) + .Where(candidate => candidate is not null) + .Take(2) + .ToList(); + + return matches.Count == 1 ? matches[0] : null; + } + + /// + /// Maps assembly name -> CLR full name ("Namespace.Type") -> type element. The assembly node and + /// the synthetic global namespace are not part of a CLR name and are skipped. + /// + private static Dictionary> BuildTypeLookup( + CodeGraph.Graph.CodeGraph graph) + { + var lookup = new Dictionary>(); + + foreach (var element in graph.Nodes.Values) + { + if (!element.IsType() || element.IsExternal) + { + continue; + } + + var path = element.GetPathToRoot(true); + if (path.Count < 2 || path[0].ElementType != CodeElementType.Assembly) + { + continue; + } + + var segments = path.Skip(1) + .Where(p => p.ElementType != CodeElementType.Namespace || + p.Name != CodeElement.GlobalNamespaceName) + .Select(p => p.Name); + + var types = lookup.TryGetValue(path[0].Name, out var existing) ? existing : lookup[path[0].Name] = []; + + // A name collision would mean two types with the same full name in one assembly, which the + // compiler would already have rejected. + types[string.Join(".", segments)] = element; + } + + return lookup; + } + + private static IEnumerable EnumerateXamlFiles(string directory) + { + if (!Directory.Exists(directory)) + { + return []; + } + + return Directory.EnumerateFiles(directory, "*.xaml", SearchOption.AllDirectories) + .Where(file => !file.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}") && + !file.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}")); + } +} diff --git a/CSharpCodeAnalyst.CodeParser/Xaml/XamlReferenceExtractor.cs b/CSharpCodeAnalyst.CodeParser/Xaml/XamlReferenceExtractor.cs new file mode 100644 index 00000000..39dbd255 --- /dev/null +++ b/CSharpCodeAnalyst.CodeParser/Xaml/XamlReferenceExtractor.cs @@ -0,0 +1,206 @@ +using System.Text.RegularExpressions; +using System.Xml; +using System.Xml.Linq; + +namespace CSharpCodeAnalyst.CodeParser.Xaml; + +/// +/// A single reference to CLR code found in a XAML file. is null when the +/// whole type is referenced (an element tag, {x:Type}), and set for {x:Static}. +/// comes from the ;assembly= part of the xmlns and is null when the +/// xmlns omits it - which means the type lives in the same assembly as the XAML file. +/// +public sealed record XamlReference( + string NamespaceName, + string TypeName, + string? MemberName, + string? AssemblyName, + int Line, + int Column) +{ + /// + /// True for an object element (<local:MyControl/>) - XAML creates an instance there, + /// so the constructor runs. False for everything that only names a type: property element syntax + /// (<local:MyControl.Items>), an attached property and {x:Type}. + /// + public bool IsInstantiation { get; init; } + + public string TypeFullName => $"{NamespaceName}.{TypeName}"; +} + +/// +/// Everything one XAML file contributes: the code-behind class it belongs to (from x:Class, null +/// for a resource dictionary) and the CLR references it makes. +/// +public sealed class XamlFileReferences +{ + public string? CodeBehindClass { get; init; } + public IReadOnlyList References { get; init; } = []; +} + +/// +/// Reads the CLR references out of a XAML file - the ones the markup compiler does *not* turn into C#. +/// +/// The WPF markup compiler generates a partial class per XAML file that contains the event handler +/// wiring and a field per x:Name, so those references are already visible to Roslyn. What +/// never reaches C# is everything declarative: it is compiled into BAML and resolved by reflection +/// at runtime. Three of those constructs carry a fully qualified CLR name and can therefore be +/// resolved exactly, which is what this extractor collects: +/// +/// +/// element tags - <local:MyControl/>, including property element syntax +/// {x:Static local:Texts.Caption} +/// {x:Type local:Foo} +/// +/// +/// {Binding Path} is deliberately NOT collected. Without evaluating the DataContext it is a +/// bare member name, and matching that by name across the whole codebase would suppress far more +/// than it explains. +/// +/// +/// Prefixes are resolved through the XML namespace declarations, so a clr-namespace xmlns is +/// mapped exactly - there is no name guessing anywhere in here. +/// +/// +public static class XamlReferenceExtractor +{ + private const string ClrNamespacePrefix = "clr-namespace:"; + private const string XamlNamespace = "http://schemas.microsoft.com/winfx/2006/xaml"; + + /// + /// Matches "{prefix:Static target:Type.Member}" and "{prefix:Type target:Type}", also when nested + /// inside another markup extension. Both prefixes are resolved against the element, never assumed. + /// + private static readonly Regex MarkupExtension = new( + @"\{\s*(?\w+)\s*:\s*(?Static|Type)\s+(?\w+)\s*:\s*(?[\w.]+)", + RegexOptions.Compiled); + + public static XamlFileReferences Extract(string xaml) + { + ArgumentNullException.ThrowIfNull(xaml); + + XDocument document; + try + { + document = XDocument.Parse(xaml, LoadOptions.SetLineInfo); + } + catch (XmlException) + { + // A malformed or unsupported file contributes nothing. It must never break the parse run. + return new XamlFileReferences(); + } + + var references = new List(); + + foreach (var element in document.Descendants()) + { + CollectElementTag(element, references); + + foreach (var attribute in element.Attributes()) + { + CollectAttachedProperty(attribute, references); + CollectMarkupExtensions(element, attribute, references); + } + } + + return new XamlFileReferences + { + CodeBehindClass = document.Root?.Attribute(XName.Get("Class", XamlNamespace))?.Value, + References = references + }; + } + + /// + /// The element tag itself: <local:MyControl/>. Property element syntax puts the property + /// behind a dot (<local:MyControl.Items>), so only the part in front of it is the type. + /// + private static void CollectElementTag(XElement element, List references) + { + // Only a tag without a dot creates an object; with one it is property element syntax. + var isInstantiation = !element.Name.LocalName.Contains('.'); + Add(element.Name.NamespaceName, element.Name.LocalName, element, references, + isInstantiation: isInstantiation); + } + + /// An attached property written as local:MyPanel.Dock="..." references MyPanel. + private static void CollectAttachedProperty(XAttribute attribute, List references) + { + Add(attribute.Name.NamespaceName, attribute.Name.LocalName, attribute, references); + } + + private static void CollectMarkupExtensions(XElement element, XAttribute attribute, + List references) + { + foreach (Match match in MarkupExtension.Matches(attribute.Value)) + { + // "x" is only a convention - verify the prefix really maps to the XAML language namespace. + var xamlNamespace = element.GetNamespaceOfPrefix(match.Groups["xamlPrefix"].Value); + if (xamlNamespace?.NamespaceName != XamlNamespace) + { + continue; + } + + var targetNamespace = element.GetNamespaceOfPrefix(match.Groups["prefix"].Value); + if (targetNamespace is null) + { + continue; + } + + var path = match.Groups["path"].Value; + if (match.Groups["kind"].Value == "Type") + { + Add(targetNamespace.NamespaceName, path, attribute, references); + continue; + } + + // {x:Static Type.Member} - the last segment is the member. + var separator = path.LastIndexOf('.'); + if (separator <= 0 || separator == path.Length - 1) + { + continue; + } + + Add(targetNamespace.NamespaceName, path[..separator], attribute, references, + path[(separator + 1)..]); + } + } + + private static void Add(string namespaceName, string localName, IXmlLineInfo position, + List references, string? memberName = null, bool isInstantiation = false) + { + if (!namespaceName.StartsWith(ClrNamespacePrefix, StringComparison.Ordinal)) + { + // A framework namespace (presentation, xaml, ...) - nothing of ours is referenced. + return; + } + + // "clr-namespace:Some.Namespace;assembly=Some.Assembly" - the assembly part is optional and + // absent exactly when the type lives in the same assembly as the XAML file. + var declaration = namespaceName[ClrNamespacePrefix.Length..]; + var semicolon = declaration.IndexOf(';'); + var clrNamespace = semicolon < 0 ? declaration : declaration[..semicolon]; + + string? assemblyName = null; + if (semicolon >= 0) + { + const string assemblyKey = "assembly="; + var assemblyPart = declaration[(semicolon + 1)..].Trim(); + if (assemblyPart.StartsWith(assemblyKey, StringComparison.Ordinal)) + { + assemblyName = assemblyPart[assemblyKey.Length..].Trim(); + } + } + + // Property element syntax: names the type in front of the dot. + var dot = localName.IndexOf('.'); + var typeName = dot < 0 ? localName : localName[..dot]; + + if (clrNamespace.Length == 0 || typeName.Length == 0) + { + return; + } + + references.Add(new XamlReference(clrNamespace, typeName, memberName, assemblyName, + position.LineNumber, position.LinePosition) { IsInstantiation = isInstantiation }); + } +} diff --git a/CSharpCodeAnalyst/App.xaml.cs b/CSharpCodeAnalyst/App.xaml.cs index 91236643..c772ad10 100644 --- a/CSharpCodeAnalyst/App.xaml.cs +++ b/CSharpCodeAnalyst/App.xaml.cs @@ -103,8 +103,12 @@ private void StartUi() // project load, read by the Method Complexity analyzer. var metricStore = new CodeGraph.Metrics.MetricStore(); + // Same shape: which members implement a contract from outside the analyzed code. Read by the + // Dead Code analyzer, which would otherwise report every framework override as unused. + var externalContractStore = new CodeGraph.Declarations.ExternalContractStore(); + var analyzerManager = new AnalyzerManager(); - analyzerManager.LoadAnalyzers(messaging, uiNotification, metricStore); + analyzerManager.LoadAnalyzers(messaging, uiNotification, metricStore, externalContractStore); var explorer = new CodeGraphExplorer(); var mainWindow = new MainWindow(); @@ -123,7 +127,7 @@ private void StartUi() var projectStorage = new JsonProjectStorage(); var projectService = new ProjectService(projectStorage, uiNotification, userSettings); - var viewModel = new MainViewModel(messaging, applicationSettings, userSettings, analyzerManager, refactoringService, projectService, metricStore); + var viewModel = new MainViewModel(messaging, applicationSettings, userSettings, analyzerManager, refactoringService, projectService, metricStore, externalContractStore); var graphViewModel = new GraphViewModel(graphViewState, explorer, messaging, applicationSettings, refactoringService); var treeViewModel = new TreeViewModel(messaging, refactoringService); var searchViewModel = new AdvancedSearchViewModel(messaging, refactoringService); diff --git a/CSharpCodeAnalyst/Features/Analyzers/AnalyzerManager.cs b/CSharpCodeAnalyst/Features/Analyzers/AnalyzerManager.cs index 29186599..34f260d0 100644 --- a/CSharpCodeAnalyst/Features/Analyzers/AnalyzerManager.cs +++ b/CSharpCodeAnalyst/Features/Analyzers/AnalyzerManager.cs @@ -1,10 +1,12 @@ using CSharpCodeAnalyst.Analyzers.EventRegistration; using CSharpCodeAnalyst.AnalyzerSdk.Contracts; using CSharpCodeAnalyst.AnalyzerSdk.Notifications; +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Metrics; using CSharpCodeAnalyst.Shared.Contracts; using CSharpCodeAnalyst.Shared.Notifications; using ArchitecturalRules = CSharpCodeAnalyst.Analyzers.ArchitecturalRules; +using DeadCode = CSharpCodeAnalyst.Analyzers.DeadCode; using MethodComplexity = CSharpCodeAnalyst.Analyzers.MethodComplexity; using SystemMetrics = CSharpCodeAnalyst.Analyzers.SystemMetrics; using TypeCohesion = CSharpCodeAnalyst.Analyzers.TypeCohesion; @@ -66,7 +68,8 @@ private void RaiseAnalyzerDataChanged() AnalyzerDataChanged?.Invoke(this, EventArgs.Empty); } - public void LoadAnalyzers(IPublisher messaging, IUserNotification userNotification, MetricStore metricStore) + public void LoadAnalyzers(IPublisher messaging, IUserNotification userNotification, MetricStore metricStore, + ExternalContractStore externalContractStore) { _analyzers.Clear(); @@ -94,6 +97,10 @@ public void LoadAnalyzers(IPublisher messaging, IUserNotification userNotificati analyzer.DataChanged += (_, _) => RaiseAnalyzerDataChanged(); _analyzers.Add(analyzer.Id, analyzer); + analyzer = new DeadCode.Analyzer(messaging, userNotification, externalContractStore); + analyzer.DataChanged += (_, _) => RaiseAnalyzerDataChanged(); + _analyzers.Add(analyzer.Id, analyzer); + } diff --git a/CSharpCodeAnalyst/Features/Tree/TreeViewModel.cs b/CSharpCodeAnalyst/Features/Tree/TreeViewModel.cs index 7ebb4fc2..adca4fc0 100644 --- a/CSharpCodeAnalyst/Features/Tree/TreeViewModel.cs +++ b/CSharpCodeAnalyst/Features/Tree/TreeViewModel.cs @@ -398,7 +398,10 @@ public void ExecuteSearch() } else { - var expr = SearchExpressionFactory.CreateSearchExpression(SearchText, SearchExpressionFactory.TextSearchField.Name); + // No negation here: SearchAndExpandNodes expands and highlights every ancestor of a match, so + // an excluding search would match nearly everything and unfold the whole tree at once. + var expr = SearchExpressionFactory.CreateSearchExpression(SearchText, + SearchExpressionFactory.TextSearchField.Name, false); SearchAndExpandNodes(TreeItems, expr); } } diff --git a/CSharpCodeAnalyst/MainViewModel.cs b/CSharpCodeAnalyst/MainViewModel.cs index b71ce85e..3a55def5 100644 --- a/CSharpCodeAnalyst/MainViewModel.cs +++ b/CSharpCodeAnalyst/MainViewModel.cs @@ -15,6 +15,7 @@ using CSharpCodeAnalyst.CodeGraph.Algorithms.Cycles; using CSharpCodeAnalyst.CodeGraph.Algorithms.Partitioning; using CSharpCodeAnalyst.CodeGraph.Graph; +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Metrics; using CSharpCodeAnalyst.CodeParser.Parser; using CSharpCodeAnalyst.CodeParser.Parser.Config; @@ -65,6 +66,7 @@ internal sealed class MainViewModel : INotifyPropertyChanged private readonly ImporterManager _importerManager = new(); private readonly MessageBus _messaging; + private readonly ExternalContractStore _externalContractStore; private readonly MetricStore _metricStore; private readonly ProjectExclusionRegExCollection _projectExclusionFilters; @@ -90,7 +92,7 @@ internal sealed class MainViewModel : INotifyPropertyChanged internal MainViewModel(MessageBus messaging, AppSettings settings, UserPreferences userSettings, AnalyzerManager analyzerManager, RefactoringService refactoringService, IProjectService projectService, - MetricStore metricStore) + MetricStore metricStore, ExternalContractStore externalContractStore) { // Initialize settings _applicationSettings = settings; @@ -98,6 +100,7 @@ internal MainViewModel(MessageBus messaging, AppSettings settings, UserPreferenc _analyzerManager = analyzerManager; _refactoringService = refactoringService; _metricStore = metricStore; + _externalContractStore = externalContractStore; analyzerManager.AnalyzerDataChanged += OnAnalyzerDataChanged; @@ -977,6 +980,7 @@ private void LoadCodeGraph(CodeGraph.Graph.CodeGraph codeGraph) Cycles = null; DynamicTabs.Clear(); _metricStore.Clear(); + _externalContractStore.Clear(); InfoPanelViewModel?.ClearQuickInfo(); UpdateStatistics(codeGraph); @@ -1026,6 +1030,9 @@ private void CompleteImport(ParseResult parseResult) // Carry the freshly collected source metrics into the shared store (empty if the option was off). _metricStore.LoadFrom(parseResult.Metrics.Metrics); + // Same for the external contracts (empty for every importer except the C# parser). + _externalContractStore.LoadFrom(parseResult.ExternalContracts.Contracts); + // Give an immediate overview of the freshly imported solution: the whole graph, every // container collapsed, so the user starts from a map instead of an empty canvas. Only on // import - loading a saved project restores the user's own view instead. Opt-out via setting. @@ -1247,6 +1254,7 @@ private ProjectData CollectProjectData() projectData.Settings.ExclusionFilter = _projectExclusionFilters.ToString(); projectData.AnalyzerData = _analyzerManager.CollectAnalyzerData(); projectData.SetMetrics(_metricStore); + projectData.SetExternalContracts(_externalContractStore); return projectData; } @@ -1284,6 +1292,7 @@ private void RestoreProjectData(ProjectData projectData) // Restore the source metrics (LoadCodeGraph cleared the shared store). // Singleton share with analyzer! _metricStore.LoadFrom(projectData.GetMetrics()); + _externalContractStore.LoadFrom(projectData.GetExternalContracts()); // Restore analyzer data _analyzerManager.RestoreAnalyzerData(projectData.AnalyzerData); diff --git a/CSharpCodeAnalyst/Persistence/Dto/ProjectData.cs b/CSharpCodeAnalyst/Persistence/Dto/ProjectData.cs index 09ca2be0..16b956a0 100644 --- a/CSharpCodeAnalyst/Persistence/Dto/ProjectData.cs +++ b/CSharpCodeAnalyst/Persistence/Dto/ProjectData.cs @@ -1,3 +1,4 @@ +using CSharpCodeAnalyst.CodeGraph.Declarations; using CSharpCodeAnalyst.CodeGraph.Graph; using CSharpCodeAnalyst.CodeGraph.Metrics; using CSharpCodeAnalyst.Features.Gallery; @@ -26,6 +27,13 @@ public class ProjectData /// public List MemberMetrics { get; set; } = []; + /// + /// Which members implement a contract from outside the analyzed code, keyed by element id. + /// Empty for every graph producer except the C# parser. An older project file simply has none, + /// and those members show up as unreferenced again until the solution is parsed anew. + /// + public Dictionary ExternalContracts { get; set; } = new(); + /// /// Gallery is already serializable. /// @@ -62,6 +70,16 @@ public Dictionary GetMetrics() }); } + public void SetExternalContracts(ExternalContractStore store) + { + ExternalContracts = store.Contracts.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + } + + public IReadOnlyDictionary GetExternalContracts() + { + return ExternalContracts; + } + /// /// Flatten the recursive structures. /// @@ -70,7 +88,7 @@ public void SetCodeGraph(CodeGraph.Graph.CodeGraph codeGraph) CodeElements = codeGraph.Nodes.Values .Select(n => new SerializableCodeElement(n.Id, n.Name, n.FullName, n.ElementType, n.SourceLocations, n.Attributes, - n.IsExternal)) + n.IsExternal, n.AccessLevel)) .ToList(); // We iterate over children, so we expect to have a parent @@ -98,7 +116,8 @@ public CodeGraph.Graph.CodeGraph GetCodeGraph() { SourceLocations = se.SourceLocations, Attributes = se.Attributes, - IsExternal = se.IsExternal + IsExternal = se.IsExternal, + AccessLevel = se.AccessLevel }; codeStructure.Nodes.Add(element.Id, element); } diff --git a/CSharpCodeAnalyst/Persistence/Dto/SerializableCodeElement.cs b/CSharpCodeAnalyst/Persistence/Dto/SerializableCodeElement.cs index 48d81f4e..4551f1c1 100644 --- a/CSharpCodeAnalyst/Persistence/Dto/SerializableCodeElement.cs +++ b/CSharpCodeAnalyst/Persistence/Dto/SerializableCodeElement.cs @@ -10,7 +10,8 @@ public class SerializableCodeElement( CodeElementType elementType, List sourceLocations, HashSet attributes, - bool isExternal = false) + bool isExternal = false, + AccessLevel accessLevel = AccessLevel.Unknown) { public string Id { get; set; } = id; public string Name { get; set; } = name; @@ -23,4 +24,10 @@ public class SerializableCodeElement( /// Whether the element belongs to a referenced assembly rather than the parsed solution. /// public bool IsExternal { get; set; } = isExternal; + + /// + /// How far the element can be reached from. Defaults to Unknown, so a project file written before + /// this existed keeps loading - the elements simply carry no visibility until the next parse. + /// + public AccessLevel AccessLevel { get; set; } = accessLevel; } diff --git a/CSharpCodeAnalyst/Resources/Strings.Designer.cs b/CSharpCodeAnalyst/Resources/Strings.Designer.cs index da564b4a..237730a3 100644 --- a/CSharpCodeAnalyst/Resources/Strings.Designer.cs +++ b/CSharpCodeAnalyst/Resources/Strings.Designer.cs @@ -2783,8 +2783,9 @@ public static string SearchingCycles_Message { /// Looks up a localized string similar to Search in code element full name. /// ///Logical operations: space = AND, '|' = OR + ///Exclude a term = prefix it with '-' (e.g. "-Strings." or "-type:property") ///Search for type = type:xxx - ///Search for internal code elements = source:intern + ///Search for internal code elements = source:intern ///Search for external code elements = source:extern ///Search with resharper style = Use at least one uppercase character in a search term.. /// @@ -3328,7 +3329,7 @@ public static string TooMuchElementsTitle { /// ///Logical operations: space = AND, '|' = OR ///Search for type = type:xxx - ///Search for internal code elements = source:intern + ///Search for internal code elements = source:intern ///Search for external code elements = source:extern ///Search with resharper style = Use at least one uppercase character in a search term.. /// diff --git a/CSharpCodeAnalyst/Resources/Strings.resx b/CSharpCodeAnalyst/Resources/Strings.resx index 44af5c86..a42b8cf4 100644 --- a/CSharpCodeAnalyst/Resources/Strings.resx +++ b/CSharpCodeAnalyst/Resources/Strings.resx @@ -675,8 +675,9 @@ If you abort the graph is maintained but not rendered. Use undo to get to the pr Search in code element full name. Logical operations: space = AND, '|' = OR +Exclude a term = prefix it with '-' (e.g. "-Strings." or "-type:property") Search for type = type:xxx -Search for internal code elements = source:intern +Search for internal code elements = source:intern Search for external code elements = source:extern Search with resharper style = Use at least one uppercase character in a search term. @@ -686,7 +687,7 @@ Search with resharper style = Use at least one uppercase character in a search t Logical operations: space = AND, '|' = OR Search for type = type:xxx -Search for internal code elements = source:intern +Search for internal code elements = source:intern Search for external code elements = source:extern Search with resharper style = Use at least one uppercase character in a search term. diff --git a/CSharpCodeAnalyst/Shared/DynamicDataGrid/DynamicDataGrid.xaml b/CSharpCodeAnalyst/Shared/DynamicDataGrid/DynamicDataGrid.xaml index 52dbaaab..81ac0c0d 100644 --- a/CSharpCodeAnalyst/Shared/DynamicDataGrid/DynamicDataGrid.xaml +++ b/CSharpCodeAnalyst/Shared/DynamicDataGrid/DynamicDataGrid.xaml @@ -3,6 +3,7 @@ xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" + xmlns:resources="clr-namespace:CSharpCodeAnalyst.Resources" mc:Ignorable="d" d:DesignHeight="300" d:DesignWidth="400"> @@ -35,6 +36,7 @@ Margin="0" Background="LightYellow" FontSize="12" + ToolTip="{x:Static resources:Strings.SearchPattern_Tooltip}" TextChanged="SearchTextBox_TextChanged" /> diff --git a/Documentation/Roslyn/corrections-and-updates.md b/Documentation/Roslyn/corrections-and-updates.md index 4ed55759..e69ddab4 100644 --- a/Documentation/Roslyn/corrections-and-updates.md +++ b/Documentation/Roslyn/corrections-and-updates.md @@ -275,3 +275,120 @@ Phase 2 now walks the declarations of **both** parts (`GetDeclaringSyntaxReferencesIncludingPartial`, using `PartialImplementationPart` / `PartialDefinitionPart`), for methods and for partial properties (C# 13) alike. The source metrics measure the implementation part. Partial *events* (C# 14) are not special-cased yet. + +## XAML: the half the markup compiler does not generate + +The WPF markup compiler writes a partial class per XAML file (`obj/.../MyView.g.cs`) and MSBuildWorkspace +runs that pass during its design-time build, so the file is part of the compilation even for a solution +that was never built. It contains the event handler wiring (`IComponentConnector.Connect`, and +`IStyleConnector.Connect` for handlers inside templates) and one field per `x:Name`. Those references are +therefore plain C# and need nothing special. + +Two things are *not* in there, and both were mistaken for dead code before: + +- Everything declarative - element tags, `{x:Static}`, `{x:Type}`, `{Binding}`, `{StaticResource}` - is + compiled into BAML and resolved by reflection at runtime. +- `x:Name` only produces a field in the file's **main name scope**. A `DataTemplate`, `ControlTemplate` or + `Style` is its own name scope and gets no field. `MainWindow.xaml` in this repository has ten `x:Name`s + and nine generated fields; the missing one sits inside a `DataTemplate`. + +So a control can be used three times in XAML, once even with a name, and produce no C# reference at all. + +A third pass (`Xaml/XamlReferenceExtractor` + `Xaml/XamlGraphLinker`, enabled by +`ParserConfig.IncludeXamlReferences`) therefore reads the XAML files next to each project and adds the +references that carry a **fully qualified CLR name**: element tags, `{x:Static}` and `{x:Type}`. Prefixes +are resolved through the `clr-namespace` xmlns declarations, so nothing is matched by guessing. The +relationships are `Uses` and carry `RelationshipAttribute.IsXamlReference`. + +`{Binding Path=...}` is deliberately left out. Without evaluating the DataContext it is a bare member name, +and matching that across the codebase would suppress far more than it explains. + +An **object element** (``) is not only a type reference - XAML creates the instance +there, so the constructor runs. The linker therefore also connects the type's `.ctor` elements. Without +that edge the constructor of a XAML-instantiated control has no incoming reference at all, and since the +body of such a control largely hangs below its constructor (`DynamicDataGrid` wires its search timer +there), everything it calls dies with it as soon as the dead code analysis cascades. Property element +syntax (``), attached properties and `{x:Type}` only name a type and are not +treated as an instantiation. Constructor overloads share the element name, so all of them are linked - +XAML picks the parameterless one, but the graph cannot tell them apart and a missing edge costs far more +than a superfluous one. + +The source of such a relationship is the code-behind class from `x:Class`. A resource dictionary has none, +so a synthetic class named after the file path takes its place - the same device already used for top-level +statements (`GlobalStatements`). It is created only when the file actually contains a resolvable reference. +Since nothing resolves the `Source` / `StartupUri` URIs of merged dictionaries, those synthetic elements +have no incoming reference and do show up in a dead code analysis; there were six of them in this +repository. + +MSBuildWorkspace note: opening a WPF project runs the markup compile through a temporary `_wpftmp.csproj`, +which can invalidate the incremental build state of the real project - a following `dotnet build` may fail +with `CS2001` for every `.g.cs` until it is rebuilt. + +## Contracts from outside the analyzed code + +A member that implements or overrides something we did not analyze - `ICommand.Execute`, +`object.GetHashCode`, `CSharpSyntaxVisitor.VisitGenericName` - has **no incoming reference anywhere in the +graph**. The framework is the caller. Every such member therefore looks like dead code, and worse: it looks +like a *confident* finding, because nothing hints at doubt. + +The relationship model cannot express it, in either configuration: + +- With `IncludeExternals` off (the default) `AddRelationshipWithFallbackToContainingType` finds neither the + member nor its containing type internally and adds **nothing at all**. There is no element to point at. +- With `IncludeExternals` on, only *types* become external elements ("Always returns the containing TYPE + element"), and member relationships are flattened to `Uses`. The result, + `VisitGenericName -Uses-> CSharpSyntaxVisitor`, is indistinguishable from a method that merely uses that + type as a parameter. Measured on this repository, turning externals on adds 954 nodes and 78 % more edges + and still does not answer the question. + +The fact is therefore recorded **beside the graph** in `ExternalContractStore` (element id -> contract +name), carried in `ParseResult` next to the source metrics. It deliberately does not live on +`CodeElement`: that type is shared with every importer, and a field only the C# parser ever fills would sit +there empty forever. `MetricStore` established the pattern - "kept beside the code graph so the graph model +stays pure". + +Two detection routes in `DeclarationAnalyzer`, both needed: + +- **`override`** - the existing hook (`methodSymbol.IsOverride`, and the property equivalent) records the + contract when the *containing type* of the overridden member is not one of ours. +- **Implicit interface implementation** - `ICommand.Execute` carries no `override` keyword, and + `AddImplementationsForInterfaceMember` only ever walks from the *interface* side, so an external interface + is never visited. `RecordExternalInterfaceImplementations` therefore walks the type's `AllInterfaces`, + skips the internal ones (those get real `Implements` edges) and resolves the rest with + `FindImplementationForInterfaceMember`. Those interfaces come from `AllInterfaces` and are already + constructed, so the definition/construction trap documented above does not apply here. + +Generic types are normalized with `OriginalDefinition` before asking whether an interface is ours - +`IHandler` is not in the map, `IHandler` is. + +The store is filled from the parallel phase 2, hence a `ConcurrentDictionary`. The dead code analysis +reports such members with a note rather than dropping them, and the fact is deliberately **not** pushed to +the containing type: implementing `IDisposable` is not a use of the class, so a class whose only remaining +trace is a `Dispose` method stays reportable. + +## Visibility on the code element + +`CodeElement.AccessLevel` carries what `ISymbol.DeclaredAccessibility` says, mapped in `HierarchyAnalyzer` +where the elements are created (types and members in one place, property accessors in another - an accessor +may narrow its property, `public int P { get; private set; }`). + +Unlike the external contracts, this belongs **on** the element rather than beside it: visibility is a +first-class property that every language the tool imports has, several consumers can use it, and the +importers can fill it later. `AccessLevel.Unknown` is the default and must always be read as "nobody told +us", never as a value - a graph from doxygen or jdeps has no visibility today, and neither has a project +file written before this existed. + +The type is called `AccessLevel`, not `Accessibility`: WPF drags a global `Accessibility` namespace into +scope, so the natural name would force a full qualification in every file of the UI projects. + +Persisted in both formats - `SerializableCodeElement` (optional constructor parameter, so old project files +keep loading) and the text serializer (`access=` written only when it is not Unknown, and an unparsable +value falls back to Unknown rather than guessing). + +The dead code analysis uses it for the confidence of a finding, and reads it over the whole containment +chain: a `public` method of an `internal` class is just as unreachable from another assembly, so it is the +*most restrictive* container that decides. + +One entry point was found only through this: a **static constructor** (`.cctor`) is run by the runtime and +never referenced from code. It is usually private, so it landed in the highest confidence band until it was +recognized as an entry point like `Main`. diff --git a/Documentation/dead-code.md b/Documentation/dead-code.md new file mode 100644 index 00000000..79025213 --- /dev/null +++ b/Documentation/dead-code.md @@ -0,0 +1,275 @@ +# Dead Code + +[TOC] + +This guide explains the **Dead Code** analysis: what it reports, what it deliberately does not report, and +how much you can trust the result. + +Available via *Analyzers → Dead Code*. The result is a sortable table: + +| Column | Meaning | +| ------- | ---------------------------------------------------------------------------- | +| Element | The fully qualified name of the unreferenced element. | +| Kind | Class, Interface, Method, Field, Property, ... — the kind of element. | +| Access | The element's visibility, empty when the producer does not supply one. | +| Confidence | How much the finding can be trusted — coloured like the complexity metric. See below. | +| Notes | Anything worth knowing about the finding. **Empty means nothing speaks against deleting it.** | + +Sort by *Notes* to get the clean cases together, and use *Jump to code* or *Copy to explorer graph* from +the context menu to check a finding. + +> **Two rows can carry the same name.** A full name is built from the plain symbol names, which carry +> neither generic arity nor a parameter list. So `WpfCommand` and `WpfCommand` read alike, and so do the +> overloads `Foo(int)` and `Foo(string)`. They are separate elements in the graph — only the display +> collides, and one of them being dead while the other is used looks like a wrong finding. *Jump to code* +> resolves it: the source locations differ. + +On a large codebase the fastest way to make the result readable is the filter box, which understands the +same expressions as the Advanced Search — including **exclusion** with a leading `-`. Whole groups of +findings disappear at once: + +``` +-Strings. -Tests -ThirdParty +``` + +`-type:property` drops a whole element kind, and `-source:extern` works as well. Terms combine with spaces +(AND) and `|` (OR); the exclusion belongs to the term it precedes. + +The *Notes* column carries two different kinds of remark, which is why it is not called something like +"might still be used": + +- A **doubt** — `Entry point`, `Test code`, `Attributes: ...`. The reference may exist somewhere the parser + cannot see, so the finding needs a second look. +- An **explanation** — `Implemented but never called: ...`, `Implements unused contract: ...`. Those are + the opposite of a doubt: the finding is well understood, and the note tells you what dies together with + it. + +## The rule + +An element is dead when **no relationship enters its subtree from the outside**. + +The subtree is the point. Two things follow from it, and both match how you would judge the code by hand: + +- A class is alive when one of its members is used, even if nothing ever names the class itself. Somebody + calls `Service.Run()`, so `Service` is not dead code — you cannot delete it. +- A class cannot keep itself alive. If its methods only call each other, every one of those calls stays + inside the subtree and proves nothing. The whole class is dead. + +Because a dead element implies a dead subtree, **only the topmost dead element is reported**. If a class is +dead you get one row for the class, not one row per method. If the class is alive but three of its methods +are unused, you get those three rows. + +Namespaces and assemblies are never reported: nothing ever references them in the graph, so they would all +look dead. Code from outside the solution (frameworks, NuGet packages) is out of scope — we see neither its +callers nor its body. + +### One round, not a chain + +The analysis reports what nothing references **right now**. It does not chase the consequences: if the only +caller of `Formatter` sits in the `Report` class you just got reported, `Formatter` is *not* also reported — +it is referenced, by dead code, but referenced. + +That is a deliberate step back from an earlier cascading version, which kept re-running with the previous +findings switched off. It worked, but it multiplied every false positive: one invisible `{Binding}` took +seven further elements with it, and the deeper rounds were only as good as the rounds below them. + +**Delete a finding and run the analysis again.** You get the next layer, one honest round at a time, and +every row stands on its own. + +### Suppressed: accessors and serialized properties + +Two things are dropped instead of being reported. Both are the same kind of noise — a getter or setter that +only a serializer, a binding or a framework ever touches. + +**A single property accessor is never a finding.** The question is whether the property is used, not +whether both halves of it are. One dead half is the normal shape of anything reflection drives: a DTO built +in C# and serialized by `System.Text.Json` has a dead *getter* on every property, and one deserialized from +JSON has a dead *setter* on every property. A property that is dead as a whole is still reported — as the +property. + +**A public property of a type carrying a serialization attribute** is not reported either. Same reason, but +it catches the case the accessor rule cannot: a property nothing touches at all from C#. + +Recognized attributes (the ones that mark the whole type): `[Serializable]`, `[DataContract]`, +`[JsonObject]`, `[JsonConverter]`, `[XmlRoot]`, `[XmlType]`, `[ProtoContract]`, `[MessagePackObject]`. None +of them is inherited in C#, so a derived DTO has to carry its own — and a plain DTO without any attribute +(`System.Text.Json` needs none) is not covered. + +Two boundaries are deliberate: + +- **Only properties, and only public ones.** A private property, a method or a field on the same type is + reported as usual — the serializer resolves by public reflection and reaches none of them. (`[Serializable]` + with `BinaryFormatter` does serialize fields; that case is not covered.) +- **Only the member.** If the whole class is dead, the class is reported — carrying a serialization + attribute is not a use of the type. + +What you lose with both rules is the finding "this property is written but never read". In code driven by +XAML and JSON that was almost pure noise; in plain logic it occasionally was not. + +### Which relationships count as a reference + +`Calls`, `Creates`, `Uses`, `Inherits`, `Invokes`, `UsesAttribute`, and `Implements` between two *types* +(`class C : IFoo` names `IFoo` in C's declaration). + +`Containment` is the parent/child hierarchy, not a use. `Bundled` is an artificial edge the graph view +creates. `Handles` points from the handler to the event and is the callback wiring, not a dependency — but +nothing is lost: registering `x.Click += OnClick` also produces a method-group `Uses` edge that keeps +`OnClick` alive. + +## Interfaces, overrides and abstract members + +`Implements` and `Overrides` between two *members* point from the implementation to the contract. Taken +literally that gives two wrong answers: an implementation never has an incoming reference and would always +look dead, while a contract member always looks used just because somebody implements it. + +So those edges are not counted as references. Instead **liveness is propagated the other way**: + +- A contract member that *is* called keeps every implementation alive — and the types holding them. + Calling `IService.Run()` is a call to `Service.Run()`, even if nothing else ever mentions `Service`. +- A contract member that is *never* called dies together with its implementations. You get one row for the + contract (`Implemented but never called: ...`) and one for each implementation (`Implements unused + contract: ...`). That pair is one of the more valuable findings: an abstraction nobody uses. + +Contracts from **outside** the solution are the exception. We cannot see who calls `IDisposable.Dispose` or +`object.ToString`, so an implementation of them is assumed to be alive and is not reported. That assumption +deliberately stops at the member: **implementing `IDisposable` is not a use of the class.** A class whose +only remaining trace is a `Dispose` method is still reported as dead. + +The graph itself cannot carry that fact. With *Include External Code* off — the default — the parser records +no `Implements` / `Overrides` relationship for a contract outside the solution, because there is no element +to point at. With it on, the edge is flattened to a `Uses` relationship on the containing *type*, which is +indistinguishable from ordinary use of that type. + +So the parser records it **beside** the graph instead, from the symbols, the same way it does for source +metrics. Those members are still listed — with `Implements external contract: ICommand.Execute` in the +*Notes* column, so the judgement stays visible instead of rows disappearing silently. + +## Confidence + +Three levels, each from one stated rule, evaluated in this order. It is a summary of what is known, not a +measurement: + +| Confidence | Rule | +| ---------- | ------ | +| **Low** (red) | The finding carries a note saying the caller may sit outside the graph — entry point, test code, attributes, an external contract. We already know we might be wrong. | +| **High** (green) | No such note, and the element **or one of its containers** is `private` or `internal`. Nothing outside the analyzed code could reach it, so "nothing references it" and "nothing *can* reference it" are the same statement. | +| **Medium** (orange) | Everything else: `public`, `protected`, or an unknown visibility. | + +The containment part matters more than it looks: a `public` method of an `internal` class cannot be called +from another assembly either, so it still qualifies as high. + +**One exception to high:** a `public` property on a type that implements `INotifyPropertyChanged` — a view +model. A XAML `{Binding}` reaches exactly that, and bindings are the one XAML construct the analysis +deliberately does not follow. The interface is recognized through the `PropertyChanged` event and is spread +down the inheritance edges, so the usual `MyViewModel : ViewModelBase` shape is covered too. + +Being confined does not help against this. A public property of an internal class cannot be referenced from +another assembly, but the binding sits *inside* the assembly and is merely invisible — a different thing. +Private, internal and protected properties stay high: the binding engine resolves by public reflection and +cannot reach them. + +> **Known gap.** The rule keys on `INotifyPropertyChanged`, so a plain object bound inside a `DataTemplate` +> is not covered. In this repository the `Mru` class (`Path`, `Command`, bound from an `ItemsSource`) is +> exactly that case and still shows as high. Recognizing it would mean demoting *every* public property, +> which costs real findings in non-WPF code. + +**Unknown visibility never reaches high.** Every importer except the C# parser leaves it unset, and so does +a project file written before this existed. That is the honest answer rather than a penalty — without +knowing the visibility we cannot claim that nothing outside could reference the element. + +The high bucket is deliberately small: it is the list you can work through without checking each entry by +hand. + +> `InternalsVisibleTo` is not taken into account. A friend assembly inside the analysis shows its references +> anyway; one outside it is the rare case this misses. + +## The notes + +The analysis can only see what the parser saw. Everything reached through reflection, dependency injection, +serialization or a test runner therefore looks unreferenced. Those elements are not silently dropped — they +are reported with a note, and you decide. (The single exception is the serialized property above, where the +note would be on every row of a DTO.) + +**Doubts** — the reference may exist where the parser cannot look: + +| Note | Meaning | +| ----------------- | ------------------------------------------------------------------------------ | +| `Entry point` | `Main`, a static constructor (the runtime runs it), or the synthetic `GlobalStatements` element for top-level statements. | +| `Test code` | The element or something below it carries a known test-framework attribute. | +| `Attributes: ...` | The element carries attributes — often the sign that a framework drives it. Every attribute is listed, including the test ones that already produced `Test code`. | + +**Explanations** — the finding is understood, and the note names what dies with it: + +| Note | Meaning | +| ----------------------------------- | ----------------------------------------------------------------------------- | +| `Implemented but never called: ...` | A contract member that is implemented but never called through the contract. | +| `Implements unused contract: ...` | Implements or overrides an internal contract member that is itself dead. | +| `Implements external contract: ...` | Implements or overrides something outside the analyzed code (`ICommand.Execute`, `object.GetHashCode`). The framework is the caller, so this is almost certainly alive. | + +Notes are collected over the whole subtree, because the evidence usually sits below what is reported: a +test fixture is reported as a dead *class*, but the `[Test]` attributes are on its methods. + +## What XAML the analysis does see + +Half of XAML is compiled into C# and is therefore fully visible; the other half is not, and the split is +sharp. + +The markup compiler writes a partial class per XAML file (`obj/.../MyView.g.cs`) and that file **is** part of +the compilation — MSBuildWorkspace runs the markup compile pass during its design-time build, so this works +even on a solution that was never built. The generated class contains a field per `x:Name`d element and a +`Connect` method that wires the event handlers: + +```csharp +this.CodeTree.ContextMenuOpening += new ContextMenuEventHandler(this.TreeView_ContextMenuOpening); +``` + +That is ordinary C#, so those references are found like any other. Everything declarative is compiled into +**BAML** instead — a binary resource resolved by reflection at runtime, with no C# generated for it. The +parser closes most of that gap by reading the XAML files themselves and adding the references that carry a +fully qualified CLR name (see `ParserConfig.IncludeXamlReferences`). + +| In XAML | Found? | +| ------------------------------------------------------------ | ------ | +| `Click="Button_Click"`, anywhere incl. templates | yes, generated C# | +| `x:Name="CodeTree"` in the file's main name scope | yes, generated field | +| `x:Name` inside `DataTemplate` / `ControlTemplate` / `Style` | no field is generated — but the element tag is read from the XAML | +| `` — the element tag | yes, read from the XAML — including the constructor it runs | +| `{x:Static resx:Strings.Header}`, `{x:Type local:Foo}` | yes, read from the XAML | +| `{Binding SaveCommand}` | **no** | +| `{StaticResource key}` | **no** | +| `Source="Styles/Buttons.xaml"`, `StartupUri` | **no** | + +Prefixes are resolved through the `clr-namespace` xmlns declarations, so nothing is matched by guessing. +`{Binding}` is deliberately left out: without evaluating the DataContext it is a bare member name, and +matching that across the codebase would suppress far more than it explains. + +The name-scope rule is worth knowing even so. `MainWindow.xaml` in this repository has ten `x:Name`s and +the generated file has nine fields — the missing one sits inside a `DataTemplate`. Before the XAML files +were read, that control looked unused despite being used three times. + +A XAML file with no code-behind class (a resource dictionary) is represented by a synthetic class named +after its path, so its references have a source. Nothing resolves the `Source` URIs of merged dictionaries, +so those synthetic elements appear in the result themselves — six of them in this repository. + +## Limitations + +Read these before deleting anything. + +- **The rest of XAML.** Most of it is covered — see the section above for the exact split. What remains + invisible is `{Binding}`, `{StaticResource}` and the `Source` / `StartupUri` URIs of merged dictionaries. + Reading the XAML files removed 187 findings on this repository when it was introduced. +- **Reflection, DI and serialization** are invisible for the same reason: the reference only exists at + runtime. What is handled explicitly are the two suppressed cases above — a single property accessor and + a public property of a type marked as a serialization target. +- **External contracts are recognized, but only for C#.** The information comes from the Roslyn symbols, so + a graph produced by one of the importers (Java, C++, Dart, ...) does not have it, and a project file + written before this existed does not either — parse the solution again to get it. +- **Public API.** Accessibility is not part of the code graph, so a library whose public API is consumed by + a different solution will report most of that API as dead. +- **Only the analyzed scope.** The analysis is only as complete as the loaded graph. If the solution was + parsed with project exclusions, or the graph came from an import, the callers may simply be missing. +- **Only one layer at a time.** Code that is kept alive solely by the code just reported does not show up + in the same run — see *One round, not a chain*. Delete and run again. +- **Dead cycles are not found.** Two classes that only use each other and nothing else each have an + incoming reference, so neither is reported — and re-running does not help, because nothing ever breaks + the cycle. Finding those requires reachability from an explicit set of entry points. diff --git a/README.md b/README.md index 2a9481aa..e2c12b21 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,7 @@ This desktop app helps you explore, understand, and manage large C# codebases, e - **Incremental Graph Exploration** – Build the exact graph you need to understand a codebase or solve a task. Map out dependencies step by step on demand (e.g., "show all incoming relationships"), and easily filter out unrelated code elements to keep the graph free of visual clutter. - **Sandbox Code Refactoring** – Experiment with structural changes and simulate cycle-breaking strategies safely, without touching your actual source code. - **Architectural Guardrails** – Define custom dependency rules and metric thresholds to actively validate and enforce a clean codebase. +- **Dead Code Detection** – Find the types and members nothing references any more, with the cases that only look unused flagged instead of hidden. - **Multi-Format Exports** – Share your architecture by exporting diagrams to PlantUML, DGML, PNG, SVG, and more. - **Git History Hotspots** – Uncover hidden technical debt by running hotspot or change-coupling analyses directly on your Git repository history. - **Design Structure Matrix (DSM)** - Explore the System's dependencies at the type level. @@ -234,6 +235,16 @@ All metrics are accessible via the Analyzer Ribbon, and the results are presente ![](Documentation/Images/metrics-example.png) +## Find dead code + +C# Code Analyst can list code elements that nothing references anymore. + +This is a heuristic only. There are many situations, like Reflection, DI, and XAML Bindings, that are not recognized. Therefore, each finding gets a confidence level. + +You can read more about the rules and the limits here: [Dead Code](Documentation/dead-code.md) + +The analysis is accessible via the Analyzer Ribbon, and the result is presented in a table on a separate tab. + ## Other languages The tool is built for C# (has its own Roslyn-based parser), but you can also import Java, C++, Python and Dart via external tools. @@ -274,6 +285,7 @@ Please keep these points in mind: - You can include external code by setting the "Include External Code" option. Only type dependencies are collected. - A method defining a lambda expression only has "uses" relationships to types and methods inside the lambda. This is because I cannot track where the lambda is actually called. I think that is a good compromise. - Primary constructors of records do not create the properties in the code graph. +- XAML Bindings are not resolved. - Projects must be loadable by the **.NET SDK's MSBuild**. Legacy non-SDK .NET Framework projects — especially old-style WPF (`net472`) — may fail to load even though they build in Visual Studio. See [Supported projects and solutions](Documentation/supported-projects.md). ## Thank you diff --git a/Tests/Helper/TestCodeGraph.cs b/Tests/Helper/TestCodeGraph.cs index b12d37e3..8cc4db4e 100644 --- a/Tests/Helper/TestCodeGraph.cs +++ b/Tests/Helper/TestCodeGraph.cs @@ -24,9 +24,10 @@ public CodeElement CreateAssembly(string id) return element; } - public CodeElement CreateClass(string id, CodeElement? parent = null, string? fullName = null) + public CodeElement CreateClass(string id, CodeElement? parent = null, string? fullName = null, + AccessLevel accessLevel = AccessLevel.Unknown) { - var element = new CodeElement(id, CodeElementType.Class, id, id, parent); + var element = new CodeElement(id, CodeElementType.Class, id, id, parent) { AccessLevel = accessLevel }; Link(parent, element); return element; } @@ -60,6 +61,14 @@ public CodeElement CreateInterface(string id, CodeElement? parent = null) return element; } + /// An interface from outside the solution (framework contract like IDisposable). + public CodeElement CreateExternalInterface(string id, CodeElement? parent = null) + { + var element = new CodeElement(id, CodeElementType.Interface, id, id, parent) { IsExternal = true }; + Link(parent, element); + return element; + } + public CodeElement CreateDelegate(string id, CodeElement? parent = null) { var element = new CodeElement(id, CodeElementType.Delegate, id, id, parent); @@ -74,16 +83,26 @@ public CodeElement CreateEvent(string id, CodeElement? parent = null) return element; } - public CodeElement CreateProperty(string id, CodeElement? parent = null) + public CodeElement CreateProperty(string id, CodeElement? parent = null, + AccessLevel accessLevel = AccessLevel.Unknown) + { + var element = new CodeElement(id, CodeElementType.Property, id, id, parent) { AccessLevel = accessLevel }; + Link(parent, element); + return element; + } + + public CodeElement CreateMethod(string id, CodeElement? parent = null, + AccessLevel accessLevel = AccessLevel.Unknown) { - var element = new CodeElement(id, CodeElementType.Property, id, id, parent); + var element = new CodeElement(id, CodeElementType.Method, id, id, parent) { AccessLevel = accessLevel }; Link(parent, element); return element; } - public CodeElement CreateMethod(string id, CodeElement? parent = null) + /// A method from outside the solution (member of a framework type). + public CodeElement CreateExternalMethod(string id, CodeElement? parent = null) { - var element = new CodeElement(id, CodeElementType.Method, id, id, parent); + var element = new CodeElement(id, CodeElementType.Method, id, id, parent) { IsExternal = true }; Link(parent, element); return element; } diff --git a/Tests/UnitTests/DeadCode/DeadCodeAnalysisTests.cs b/Tests/UnitTests/DeadCode/DeadCodeAnalysisTests.cs new file mode 100644 index 00000000..b6904d3e --- /dev/null +++ b/Tests/UnitTests/DeadCode/DeadCodeAnalysisTests.cs @@ -0,0 +1,418 @@ +using CodeParserTests.Helper; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; +using CSharpCodeAnalyst.CodeGraph.Declarations; +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CodeParserTests.UnitTests.DeadCode; + +[TestFixture] +public class DeadCodeAnalysisTests +{ + [SetUp] + public void SetUp() + { + _graph = new TestCodeGraph(); + } + + private TestCodeGraph _graph = null!; + + private void Rel(CodeElement source, CodeElement target, RelationshipType type) + { + source.Relationships.Add(new Relationship(source.Id, target.Id, type)); + } + + private string[] Reported() + { + return DeadCodeAnalysis.Calculate(_graph) + .Select(f => f.Element.FullName) + .ToArray(); + } + + private DeadCodeFinding FindingFor(CodeElement element) + { + return DeadCodeAnalysis.Calculate(_graph).Single(f => f.Element.Id == element.Id); + } + + [Test] + public void Calculate_EmptyGraph_NoFindings() + { + Assert.That(DeadCodeAnalysis.Calculate(_graph), Is.Empty); + } + + [Test] + public void Calculate_UnreferencedClass_Reported() + { + _graph.CreateClass("A"); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "A" })); + } + + [Test] + public void Calculate_ClassUsedByAnotherClass_NotReported() + { + // B uses A, so only B itself has no incoming reference. + var a = _graph.CreateClass("A"); + var b = _graph.CreateClass("B"); + Rel(b, a, RelationshipType.Uses); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "B" })); + } + + [Test] + public void Calculate_MethodCalledFromOutside_KeepsWholeClassAlive() + { + // Nothing names A, but B.M calls A.M -> the reference enters A's subtree from the outside. + var a = _graph.CreateClass("A"); + var am = _graph.CreateMethod("A.M", a); + var b = _graph.CreateClass("B"); + var bm = _graph.CreateMethod("B.M", b); + Rel(bm, am, RelationshipType.Calls); + + // B is dead; B.M is inside it and rolled up. + Assert.That(Reported(), Is.EquivalentTo(new[] { "B" })); + } + + [Test] + public void Calculate_ClassWithOnlyInternalCalls_ReportedAsOneFinding() + { + // A.M1 -> A.M2 does not prove anything about A: the reference never leaves the subtree. + var a = _graph.CreateClass("A"); + var m1 = _graph.CreateMethod("A.M1", a); + var m2 = _graph.CreateMethod("A.M2", a); + Rel(m1, m2, RelationshipType.Calls); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "A" })); + } + + [Test] + public void Calculate_UnusedMemberOfLiveClass_Reported() + { + var a = _graph.CreateClass("A"); + var used = _graph.CreateMethod("A.Used", a); + _graph.CreateMethod("A.Unused", a); + var b = _graph.CreateClass("B"); + var bm = _graph.CreateMethod("B.M", b); + Rel(bm, used, RelationshipType.Calls); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "A.Unused", "B" })); + } + + [Test] + public void Calculate_NestedDeadClass_OnlyOutermostReported() + { + var outer = _graph.CreateClass("Outer"); + var inner = _graph.CreateClass("Outer.Inner", outer); + _graph.CreateMethod("Outer.Inner.M", inner); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Outer" })); + } + + [Test] + public void Calculate_ContainersAndExternalElements_NeverReported() + { + var assembly = _graph.CreateAssembly("Asm"); + var ns = _graph.CreateNamespace("Asm.Ns", assembly); + _graph.CreateClass("Asm.Ns.A", ns); + _graph.CreateExternalClass("Ext"); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Asm.Ns.A" })); + } + + [Test] + public void Calculate_ContainmentAndHandles_AreNoReferences() + { + // Handles points handler -> event; it is the callback wiring, not a use of the handler. + var publisher = _graph.CreateClass("Publisher"); + var evt = _graph.CreateEvent("Publisher.Changed", publisher); + var subscriber = _graph.CreateClass("Subscriber"); + var handler = _graph.CreateMethod("Subscriber.OnChanged", subscriber); + Rel(handler, evt, RelationshipType.Handles); + Rel(publisher, subscriber, RelationshipType.Containment); + + // The Handles edge enters Publisher's subtree and the Containment edge enters Subscriber's, + // yet neither is a reference - both classes stay dead. + Assert.That(Reported(), Is.EquivalentTo(new[] { "Publisher", "Subscriber" })); + } + + [Test] + public void Calculate_CalledThroughInterface_KeepsImplementationAndItsTypeAlive() + { + var contract = _graph.CreateInterface("IFoo"); + var contractMember = _graph.CreateMethod("IFoo.Bar", contract); + var impl = _graph.CreateClass("C"); + var implMember = _graph.CreateMethod("C.Bar", impl); + Rel(impl, contract, RelationshipType.Implements); + Rel(implMember, contractMember, RelationshipType.Implements); + + var user = _graph.CreateClass("User"); + var userMethod = _graph.CreateMethod("User.M", user); + Rel(userMethod, contractMember, RelationshipType.Calls); + + // Nobody creates C, yet the call through IFoo.Bar reaches C.Bar - so neither is dead. + Assert.That(Reported(), Is.EquivalentTo(new[] { "User" })); + } + + [Test] + public void Calculate_OverrideChain_LivenessPropagatesTransitively() + { + var contract = _graph.CreateInterface("IFoo"); + var contractMember = _graph.CreateMethod("IFoo.Bar", contract); + var middle = _graph.CreateClass("Base"); + var middleMember = _graph.CreateMethod("Base.Bar", middle); + var leaf = _graph.CreateClass("Derived"); + var leafMember = _graph.CreateMethod("Derived.Bar", leaf); + + Rel(middle, contract, RelationshipType.Implements); + Rel(middleMember, contractMember, RelationshipType.Implements); + Rel(leaf, middle, RelationshipType.Inherits); + Rel(leafMember, middleMember, RelationshipType.Overrides); + + var user = _graph.CreateClass("User"); + Rel(user, contractMember, RelationshipType.Calls); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "User" })); + } + + [Test] + public void Calculate_ContractImplementedButNeverCalled_ContractAndImplementationReported() + { + var contract = _graph.CreateInterface("IFoo"); + var contractMember = _graph.CreateMethod("IFoo.Bar", contract); + var impl = _graph.CreateClass("C"); + var implMember = _graph.CreateMethod("C.Bar", impl); + Rel(impl, contract, RelationshipType.Implements); + Rel(implMember, contractMember, RelationshipType.Implements); + + // C is instantiated, so the class itself is alive - but nobody ever calls Bar. + var user = _graph.CreateClass("User"); + Rel(user, impl, RelationshipType.Creates); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "C.Bar", "IFoo.Bar", "User" })); + + var contractFinding = FindingFor(contractMember); + Assert.That(contractFinding.Hints.HasFlag(DeadCodeHint.ContractNeverCalled), Is.True); + Assert.That(contractFinding.RelatedMembers.Select(m => m.FullName), Is.EquivalentTo(new[] { "C.Bar" })); + + var implFinding = FindingFor(implMember); + Assert.That(implFinding.Hints.HasFlag(DeadCodeHint.ImplementsDeadContract), Is.True); + Assert.That(implFinding.RelatedMembers.Select(m => m.FullName), Is.EquivalentTo(new[] { "IFoo.Bar" })); + } + + [Test] + public void Calculate_ImplementsExternalContract_MemberAliveButClassStillDead() + { + // class C : IDisposable { public void Dispose() {} } - Dispose is called by code we cannot see, + // but implementing IDisposable is no use of C itself. + var external = _graph.CreateExternalInterface("IDisposable"); + var externalMember = _graph.CreateExternalMethod("IDisposable.Dispose", external); + var impl = _graph.CreateClass("C"); + var implMember = _graph.CreateMethod("C.Dispose", impl); + Rel(impl, external, RelationshipType.Implements); + Rel(implMember, externalMember, RelationshipType.Implements); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "C" })); + } + + [Test] + public void Calculate_OverridesUnresolvedBaseMember_ReportedWithTheExternalContractHint() + { + // The parser falls back to the containing type when it cannot resolve the exact base member + // (generic base methods). We cannot tell who calls it - the member is still reported, but the + // note says why it is probably alive rather than dropping the row silently. + var baseClass = _graph.CreateClass("Base"); + var derived = _graph.CreateClass("Derived"); + var member = _graph.CreateMethod("Derived.M", derived); + Rel(derived, baseClass, RelationshipType.Inherits); + Rel(member, baseClass, RelationshipType.Overrides); + + var user = _graph.CreateClass("User"); + Rel(user, derived, RelationshipType.Creates); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Derived.M", "User" })); + + var finding = FindingFor(member); + Assert.Multiple(() => + { + Assert.That(finding.Hints.HasFlag(DeadCodeHint.ImplementsExternalContract), Is.True); + Assert.That(finding.ExternalContract, Is.EqualTo("Base")); + }); + } + + [Test] + public void Calculate_ExternalContractFromTheStore_ReportedWithTheContractName() + { + // The usual case: the parser recorded the contract beside the graph because there is no element + // to point an edge at (IncludeExternals is off, so ICommand is not in the graph at all). + var live = _graph.CreateClass("Command"); + var execute = _graph.CreateMethod("Command.Execute", live); + var user = _graph.CreateClass("User"); + Rel(user, live, RelationshipType.Creates); + + var store = new ExternalContractStore(); + store.Add(execute.Id, "ICommand.Execute"); + + var findings = DeadCodeAnalysis.Calculate(_graph, store); + var finding = findings.Single(f => f.Element.Id == execute.Id); + + Assert.Multiple(() => + { + Assert.That(finding.Hints.HasFlag(DeadCodeHint.ImplementsExternalContract), Is.True); + Assert.That(finding.ExternalContract, Is.EqualTo("ICommand.Execute")); + + // The class itself is untouched by the assumption - it is created, so it is not reported. + Assert.That(findings.Select(f => f.Element.FullName), Does.Not.Contain("Command")); + }); + } + + [Test] + public void Calculate_EntryPointAndTestCode_ReportedWithHint() + { + var program = _graph.CreateClass("Program"); + _graph.CreateMethod("Main", program); + + var fixture = _graph.CreateClass("MyTests"); + var testMethod = _graph.CreateMethod("MyTests.ShouldWork", fixture); + testMethod.Attributes.Add("TestAttribute"); + + var service = _graph.CreateClass("Service"); + service.Attributes.Add("ObsoleteAttribute"); + + Assert.That(FindingFor(program).Hints, Is.EqualTo(DeadCodeHint.EntryPoint)); + Assert.That(FindingFor(fixture).Hints, Is.EqualTo(DeadCodeHint.TestCode)); + Assert.That(FindingFor(service).Hints, Is.EqualTo(DeadCodeHint.Attributed)); + Assert.That(FindingFor(service).Attributes, Is.EquivalentTo(new[] { "ObsoleteAttribute" })); + } + + [Test] + public void Calculate_UnusedPropertyAccessor_NotReported() + { + var a = _graph.CreateClass("A"); + var property = _graph.CreateProperty("A.Value", a); + var getter = _graph.CreatePropertyAccessor("A.get_Value", property); + _graph.CreatePropertyAccessor("A.set_Value", property); + + var user = _graph.CreateClass("User"); + Rel(user, getter, RelationshipType.Calls); + + // The setter alone is no finding: the question is whether the property is used, and it is. One + // unused half is the normal shape of everything a serializer, a binding or a framework drives. + Assert.That(Reported(), Is.EquivalentTo(new[] { "User" })); + } + + [Test] + public void Calculate_PropertyWithNoUsedAccessor_ReportedAsTheProperty() + { + var a = _graph.CreateClass("A"); + var property = _graph.CreateProperty("A.Value", a); + _graph.CreatePropertyAccessor("A.get_Value", property); + _graph.CreatePropertyAccessor("A.set_Value", property); + + var used = _graph.CreateMethod("A.Used", a); + var user = _graph.CreateClass("User"); + Rel(user, used, RelationshipType.Calls); + + // Suppressing the accessors must not hide a property that is dead as a whole - it rolls up. + Assert.That(Reported(), Is.EquivalentTo(new[] { "A.Value", "User" })); + } + + [Test] + public void Calculate_CodeOnlyUsedByDeadCode_NotReported() + { + // Deliberate: the analysis reports what nothing references right now and does not chase the + // consequences. Formatter is referenced - by dead code, but referenced. Delete Report and run the + // analysis again, and Formatter shows up. That keeps every finding standing on its own instead of + // stacking on the round below it. + var report = _graph.CreateClass("Report"); + var print = _graph.CreateMethod("Report.Print", report); + var formatter = _graph.CreateClass("Formatter"); + var format = _graph.CreateMethod("Formatter.Format", formatter); + Rel(print, format, RelationshipType.Calls); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Report" })); + } + + [Test] + public void Calculate_MutualReference_NotFound() + { + // The known limit: two elements that only reference each other keep each other alive. Finding + // those needs reachability from an explicit set of entry points. + var a = _graph.CreateClass("A"); + var am = _graph.CreateMethod("A.M", a); + var b = _graph.CreateClass("B"); + var bm = _graph.CreateMethod("B.M", b); + Rel(am, bm, RelationshipType.Calls); + Rel(bm, am, RelationshipType.Calls); + + Assert.That(Reported(), Is.Empty); + } + + /// + /// A type a serializer drives, kept alive by a user so that the members are reported individually. + /// + private CodeElement CreateSerializableType(string attribute = "DataContractAttribute") + { + var type = _graph.CreateClass("Config"); + type.Attributes.Add(attribute); + + var user = _graph.CreateClass("User"); + Rel(user, type, RelationshipType.Creates); + return type; + } + + [Test] + public void Calculate_PublicPropertyOfASerializableType_NotReported() + { + // The serializer reads it by reflection. On such a type every property looks dead, so reporting + // them would only fill the result with rows nobody can act on. + var type = CreateSerializableType(); + _graph.CreateProperty("Config.Title", type, AccessLevel.Public); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "User" })); + } + + [Test] + public void Calculate_NonPublicPropertyOfASerializableType_Reported() + { + // Out of reach for the serializer, which resolves by public reflection. + var type = CreateSerializableType("SerializableAttribute"); + _graph.CreateProperty("Config.Secret", type, AccessLevel.Private); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Config.Secret", "User" })); + } + + [Test] + public void Calculate_MethodOfASerializableType_Reported() + { + // The exception is about the serialized state, not about everything on the type. + var type = CreateSerializableType(); + _graph.CreateMethod("Config.Validate", type, AccessLevel.Public); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Config.Validate", "User" })); + } + + [Test] + public void Calculate_PublicPropertyOfAnOrdinaryType_Reported() + { + // Without one of the serialization attributes there is nothing to suspect. + var type = _graph.CreateClass("Config"); + _graph.CreateProperty("Config.Title", type, AccessLevel.Public); + + var user = _graph.CreateClass("User"); + Rel(user, type, RelationshipType.Creates); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "Config.Title", "User" })); + } + + [Test] + public void Calculate_PropertyOfASerializableTypeWithNoUsedAccessor_NotReported() + { + // Here the accessor roll-up alone would not help: nothing touches the property at all, so without + // the serialization rule it would be reported as a dead property. + var type = CreateSerializableType(); + var property = _graph.CreateProperty("Config.Title", type, AccessLevel.Public); + _graph.CreatePropertyAccessor("Config.get_Title", property); + _graph.CreatePropertyAccessor("Config.set_Title", property); + + Assert.That(Reported(), Is.EquivalentTo(new[] { "User" })); + } +} diff --git a/Tests/UnitTests/DeadCode/DeadCodeConfidenceTests.cs b/Tests/UnitTests/DeadCode/DeadCodeConfidenceTests.cs new file mode 100644 index 00000000..cd8e32f3 --- /dev/null +++ b/Tests/UnitTests/DeadCode/DeadCodeConfidenceTests.cs @@ -0,0 +1,217 @@ +using CodeParserTests.Helper; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; +using CSharpCodeAnalyst.CodeGraph.Declarations; +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CodeParserTests.UnitTests.DeadCode; + +/// +/// The confidence of a finding follows three rules: a note about a caller outside the graph makes it +/// low, a direct finding confined to the analyzed code makes it high, everything else is medium. +/// +[TestFixture] +public class DeadCodeConfidenceTests +{ + [SetUp] + public void SetUp() + { + _graph = new TestCodeGraph(); + } + + private TestCodeGraph _graph = null!; + + private void Rel(CodeElement source, CodeElement target, RelationshipType type) + { + source.Relationships.Add(new Relationship(source.Id, target.Id, type)); + } + + private DeadCodeConfidence ConfidenceOf(CodeElement element, ExternalContractStore? store = null) + { + return DeadCodeAnalysis.Calculate(_graph, store).Single(f => f.Element.Id == element.Id).Confidence; + } + + /// A live class holding one used and one unused member - the unused one is the finding. + private CodeElement CreateUnusedMember(AccessLevel memberAccess, AccessLevel classAccess = AccessLevel.Public) + { + var type = _graph.CreateClass("Widget", accessLevel: classAccess); + var used = _graph.CreateMethod("Widget.Used", type, AccessLevel.Public); + var unused = _graph.CreateMethod("Widget.Unused", type, memberAccess); + + var program = _graph.CreateClass("Program"); + var main = _graph.CreateMethod("Main", program); + Rel(main, used, RelationshipType.Calls); + + return unused; + } + + [Test] + public void PrivateMember_IsHighConfidence() + { + // Nothing outside the type could call it, and nothing inside does. + Assert.That(ConfidenceOf(CreateUnusedMember(AccessLevel.Private)), Is.EqualTo(DeadCodeConfidence.High)); + } + + [Test] + public void InternalMember_IsHighConfidence() + { + Assert.That(ConfidenceOf(CreateUnusedMember(AccessLevel.Internal)), Is.EqualTo(DeadCodeConfidence.High)); + } + + [Test] + public void PublicMember_IsMediumConfidence() + { + // A caller could sit in code we never analyzed. + Assert.That(ConfidenceOf(CreateUnusedMember(AccessLevel.Public)), Is.EqualTo(DeadCodeConfidence.Medium)); + } + + [Test] + public void ProtectedMember_IsMediumConfidence() + { + // A derived class in another assembly could override or call it. + Assert.That(ConfidenceOf(CreateUnusedMember(AccessLevel.Protected)), Is.EqualTo(DeadCodeConfidence.Medium)); + } + + [Test] + public void UnknownVisibility_IsMediumConfidence() + { + // What every importer other than the C# parser produces. "No information" must not be read as + // "confined", so the finding cannot reach the top level. + Assert.That(ConfidenceOf(CreateUnusedMember(AccessLevel.Unknown)), Is.EqualTo(DeadCodeConfidence.Medium)); + } + + [Test] + public void PublicMemberOfAnInternalClass_IsHighConfidence() + { + // The effective reach is what counts: a public method of an internal class cannot be called from + // another assembly either. + var unused = CreateUnusedMember(AccessLevel.Public, AccessLevel.Internal); + + Assert.That(ConfidenceOf(unused), Is.EqualTo(DeadCodeConfidence.High)); + } + + [Test] + public void NoteAboutACallerOutsideTheGraph_IsLowConfidence() + { + var type = _graph.CreateClass("Command", accessLevel: AccessLevel.Internal); + var used = _graph.CreateMethod("Command.Used", type, AccessLevel.Private); + var execute = _graph.CreateMethod("Command.Execute", type, AccessLevel.Private); + + var program = _graph.CreateClass("Program"); + var main = _graph.CreateMethod("Main", program); + Rel(main, used, RelationshipType.Calls); + + var store = new ExternalContractStore(); + store.Add(execute.Id, "ICommand.Execute"); + + // Private and internal would say "high", but we know the framework calls it - the note wins. + Assert.That(ConfidenceOf(execute, store), Is.EqualTo(DeadCodeConfidence.Low)); + } + + /// + /// A view model with one used and one unused property. The store entry is what tells the analysis + /// that the type raises change notifications - the interface itself is not in the graph. + /// + private (CodeElement Unused, ExternalContractStore Store) CreateViewModel(AccessLevel propertyAccess, + AccessLevel typeAccess = AccessLevel.Internal) + { + var viewModel = _graph.CreateClass("MainViewModel", accessLevel: typeAccess); + var changed = _graph.CreateEvent("MainViewModel.PropertyChanged", viewModel); + var used = _graph.CreateMethod("MainViewModel.Used", viewModel, AccessLevel.Public); + var unused = _graph.CreateProperty("MainViewModel.Title", viewModel); + unused = Retype(unused, propertyAccess); + + var program = _graph.CreateClass("Program"); + var main = _graph.CreateMethod("Main", program); + Rel(main, used, RelationshipType.Calls); + + var store = new ExternalContractStore(); + store.Add(changed.Id, "INotifyPropertyChanged.PropertyChanged"); + return (unused, store); + } + + /// TestCodeGraph has no accessibility overload for properties - replace the element. + private CodeElement Retype(CodeElement element, AccessLevel accessLevel) + { + var replacement = new CodeElement(element.Id, element.ElementType, element.Name, element.FullName, + element.Parent) { AccessLevel = accessLevel }; + element.Parent?.Children.Remove(element); + element.Parent?.Children.Add(replacement); + _graph.Nodes[replacement.Id] = replacement; + return replacement; + } + + [Test] + public void PublicPropertyOnANotifyingType_IsNotHighConfidence() + { + // A XAML {Binding} reaches exactly this and is invisible to the analysis. + var (unused, store) = CreateViewModel(AccessLevel.Public); + + Assert.That(ConfidenceOf(unused, store), Is.EqualTo(DeadCodeConfidence.Medium)); + } + + [Test] + public void PrivatePropertyOnANotifyingType_StaysHighConfidence() + { + // The binding engine resolves by public reflection, so it can never reach this one. + var (unused, store) = CreateViewModel(AccessLevel.Private); + + Assert.That(ConfidenceOf(unused, store), Is.EqualTo(DeadCodeConfidence.High)); + } + + [Test] + public void PublicPropertyOnAnOrdinaryType_StaysHighConfidence() + { + // Without the notification contract there is no reason to suspect a binding. + var type = _graph.CreateClass("Options", accessLevel: AccessLevel.Internal); + var used = _graph.CreateMethod("Options.Used", type, AccessLevel.Public); + var unused = Retype(_graph.CreateProperty("Options.Title", type), AccessLevel.Public); + + var program = _graph.CreateClass("Program"); + var main = _graph.CreateMethod("Main", program); + Rel(main, used, RelationshipType.Calls); + + Assert.That(ConfidenceOf(unused), Is.EqualTo(DeadCodeConfidence.High)); + } + + [Test] + public void PublicPropertyOnADerivedViewModel_IsNotHighConfidence() + { + // The common MVVM shape: the base class implements the interface, the derived one inherits it. + // Without following the Inherits edge this case would be missed. + var (_, store) = CreateViewModel(AccessLevel.Private); + var baseType = _graph.Nodes.Values.Single(n => n.Name == "MainViewModel"); + + var derived = _graph.CreateClass("DetailViewModel", accessLevel: AccessLevel.Internal); + var unused = Retype(_graph.CreateProperty("DetailViewModel.Caption", derived), AccessLevel.Public); + Rel(derived, baseType, RelationshipType.Inherits); + + // Keep the derived class itself alive, otherwise it is reported and swallows the property. + var main = _graph.Nodes.Values.Single(n => n.Name == "Main"); + Rel(main, derived, RelationshipType.Creates); + + Assert.That(ConfidenceOf(unused, store), Is.EqualTo(DeadCodeConfidence.Medium)); + } + + [Test] + public void StaticConstructor_IsAnEntryPointAndNotHighConfidence() + { + // The runtime runs it before the first use of the type; nothing in the code references it. It is + // usually private, so without the entry point rule it would land in the highest confidence band. + var type = _graph.CreateClass("Cache", accessLevel: AccessLevel.Internal); + var staticConstructor = _graph.CreateMethod(".cctor", type, AccessLevel.Private); + var used = _graph.CreateMethod("Cache.Used", type, AccessLevel.Public); + + var program = _graph.CreateClass("Program"); + var main = _graph.CreateMethod("Main", program); + Rel(main, used, RelationshipType.Calls); + + var finding = DeadCodeAnalysis.Calculate(_graph).Single(f => f.Element.Id == staticConstructor.Id); + + Assert.Multiple(() => + { + Assert.That(finding.Hints.HasFlag(DeadCodeHint.EntryPoint), Is.True); + Assert.That(finding.Confidence, Is.EqualTo(DeadCodeConfidence.Low)); + }); + } + +} diff --git a/Tests/UnitTests/DeadCode/DeadCodeParseTests.cs b/Tests/UnitTests/DeadCode/DeadCodeParseTests.cs new file mode 100644 index 00000000..f7c57547 --- /dev/null +++ b/Tests/UnitTests/DeadCode/DeadCodeParseTests.cs @@ -0,0 +1,100 @@ +using CodeParserTests.UnitTests.Parser; +using CSharpCodeAnalyst.CodeGraph.Algorithms.DeadCode; + +namespace CodeParserTests.UnitTests.DeadCode; + +/// +/// End-to-end check of the dead code analysis against a real parse result: the synthetic graph tests +/// pin the rules, this fixture proves the rules match what the parser actually produces. +/// +[TestFixture] +public class DeadCodeParseTests : InMemoryParseTestBase +{ + protected override string Code => """ + namespace Demo; + + public interface IService + { + void Run(); + void NeverCalled(); + } + + public class Service : IService + { + private readonly int _used = 1; + private readonly int _unused = 2; + + public void Run() { Helper.Help(_used); } + public void NeverCalled() { } + private void PrivateUnused() { } + } + + public static class Helper + { + public static void Help(int x) { } + public static void UnusedHelp() { } + } + + public class DeadClass + { + public void A() { B(); } + private void B() { } + } + + public class Program + { + public static void Main() + { + IService service = new Service(); + service.Run(); + } + } + """; + + private string[] Reported() + { + return DeadCodeAnalysis.Calculate(Graph).Select(f => PathOf(f.Element)).ToArray(); + } + + [Test] + public void Calculate_ReportsExactlyTheUnreferencedElements() + { + // Service and Helper are reached from Main, IService.Run through the interface call. + // DeadClass only calls itself, Program is only the entry point holder. + Assert.That(Reported(), Is.EquivalentTo(new[] + { + "DeadClass", + "Program", + "Helper.UnusedHelp", + "IService.NeverCalled", + "Service.NeverCalled", + "Service.PrivateUnused", + "Service._unused" + })); + } + + [Test] + public void Calculate_MainHolder_CarriesEntryPointHint() + { + var program = DeadCodeAnalysis.Calculate(Graph).Single(f => PathOf(f.Element) == "Program"); + + Assert.That(program.Hints.HasFlag(DeadCodeHint.EntryPoint), Is.True); + } + + [Test] + public void Calculate_UncalledContract_LinksToItsImplementation() + { + var findings = DeadCodeAnalysis.Calculate(Graph); + var contract = findings.Single(f => PathOf(f.Element) == "IService.NeverCalled"); + var implementation = findings.Single(f => PathOf(f.Element) == "Service.NeverCalled"); + + Assert.Multiple(() => + { + Assert.That(contract.Hints.HasFlag(DeadCodeHint.ContractNeverCalled), Is.True); + Assert.That(contract.RelatedMembers.Select(PathOf), Is.EquivalentTo(new[] { "Service.NeverCalled" })); + + Assert.That(implementation.Hints.HasFlag(DeadCodeHint.ImplementsDeadContract), Is.True); + Assert.That(implementation.RelatedMembers.Select(PathOf), Is.EquivalentTo(new[] { "IService.NeverCalled" })); + }); + } +} diff --git a/Tests/UnitTests/Parser/ExternalContractParseTests.cs b/Tests/UnitTests/Parser/ExternalContractParseTests.cs new file mode 100644 index 00000000..bcac07f2 --- /dev/null +++ b/Tests/UnitTests/Parser/ExternalContractParseTests.cs @@ -0,0 +1,142 @@ +using CSharpCodeAnalyst.CodeGraph.Declarations; +using CSharpCodeAnalyst.CodeGraph.Graph; +using CSharpCodeAnalyst.CodeParser.Parser.Config; + +namespace CodeParserTests.UnitTests.Parser; + +/// +/// A member that implements or overrides something from outside the analyzed code has no incoming +/// reference anywhere in the graph - the framework is the caller. The relationship model cannot carry +/// the fact, so the parser records it in the beside the graph. +/// This fixture pins both routes: the "override" keyword and an implicit interface implementation. +/// +[TestFixture] +public class ExternalContractParseTests +{ + [OneTimeSetUp] + public async Task ParseCode() + { + const string code = """ + using System; + using System.Collections; + + namespace Demo; + + public interface IOwn + { + void Handle(); + } + + public class Widget : IDisposable, IOwn + { + // Implicit implementation of a framework interface - no "override" keyword. + public void Dispose() { } + + // Implementation of one of our own interfaces: a real Implements edge exists. + public void Handle() { } + + // Overrides a framework member. + public override string ToString() => "widget"; + + // Neither. Nothing keeps this alive. + public void Unused() { } + + // Overrides a framework member declared as a property. + public override int GetHashCode() => 0; + } + + public abstract class OwnBase + { + public abstract void Run(); + } + + public class OwnDerived : OwnBase + { + // Overrides one of ours: a real Overrides edge exists. + public override void Run() { } + } + + public class Sequence : IEnumerable + { + public IEnumerator GetEnumerator() => throw new NotImplementedException(); + } + """; + + var parser = new CSharpCodeAnalyst.CodeParser.Parser.Parser( + new ParserConfig(new ProjectExclusionRegExCollection(), false)); + var result = await parser.ParseSourceAsync(code); + + _graph = result.CodeGraph; + _contracts = result.ExternalContracts; + } + + private CodeGraph _graph = null!; + private ExternalContractStore _contracts = null!; + + private string? ContractOf(string path) + { + var element = _graph.Nodes.Values.Single(n => PathOf(n) == path); + return _contracts.TryGet(element.Id); + } + + private static string PathOf(CodeElement element) + { + var parts = new List(); + var current = element; + while (current is not null && current.ElementType is not (CodeElementType.Namespace or CodeElementType.Assembly)) + { + parts.Insert(0, current.Name); + current = current.Parent; + } + + return string.Join(".", parts); + } + + [Test] + public void ImplicitImplementationOfAFrameworkInterface_IsRecorded() + { + Assert.That(ContractOf("Widget.Dispose"), Is.EqualTo("IDisposable.Dispose")); + } + + [Test] + public void OverrideOfAFrameworkMember_IsRecorded() + { + Assert.Multiple(() => + { + Assert.That(ContractOf("Widget.ToString"), Is.EqualTo("Object.ToString")); + Assert.That(ContractOf("Widget.GetHashCode"), Is.EqualTo("Object.GetHashCode")); + }); + } + + [Test] + public void ImplementationOfAFrameworkInterfaceOnAnotherType_IsRecorded() + { + Assert.That(ContractOf("Sequence.GetEnumerator"), Is.EqualTo("IEnumerable.GetEnumerator")); + } + + [Test] + public void ImplementationOfOurOwnInterface_IsNotRecorded() + { + // The graph already has the Implements edge; the dead code analysis propagates liveness along it. + Assert.That(ContractOf("Widget.Handle"), Is.Null); + } + + [Test] + public void OverrideOfOurOwnBaseClass_IsNotRecorded() + { + Assert.That(ContractOf("OwnDerived.Run"), Is.Null); + } + + [Test] + public void OrdinaryMember_IsNotRecorded() + { + Assert.That(ContractOf("Widget.Unused"), Is.Null); + } + + [Test] + public void TheTypeItself_IsNeverRecorded() + { + // Implementing IDisposable is not a use of the class - it must still be reportable as dead code. + Assert.That(ContractOf("Widget"), Is.Null); + } +} diff --git a/Tests/UnitTests/Search/SearchExpressionTests.cs b/Tests/UnitTests/Search/SearchExpressionTests.cs new file mode 100644 index 00000000..01052196 --- /dev/null +++ b/Tests/UnitTests/Search/SearchExpressionTests.cs @@ -0,0 +1,239 @@ +using CSharpCodeAnalyst.AnalyzerSdk.Search; +using CSharpCodeAnalyst.CodeGraph.Graph; + +namespace CodeParserTests.UnitTests.Search; + +/// +/// Pins the search grammar reachable through : AND (space), +/// OR (pipe), negation (leading minus) and the "type:" / "source:" terms. The same expression feeds +/// the tree, the graph search, the Advanced Search and every analyzer table, so the behaviour is +/// shared by all of them. +/// +[TestFixture] +public class SearchExpressionTests +{ + private static CodeElement Element(string fullName, string? name = null, + CodeElementType type = CodeElementType.Class, bool isExternal = false) + { + return new CodeElement(fullName, type, name ?? fullName, fullName, null) { IsExternal = isExternal }; + } + + private static bool Matches(string search, CodeElement? element, + SearchExpressionFactory.TextSearchField field = SearchExpressionFactory.TextSearchField.FullName) + { + return SearchExpressionFactory.CreateSearchExpression(search, field).Evaluate(element); + } + + [Test] + public void Term_MatchesSubstringOfTheFullName() + { + var element = Element("App.Resources.Strings.Close_Button"); + + Assert.Multiple(() => + { + Assert.That(Matches("resources", element), Is.True); + Assert.That(Matches("missing", element), Is.False); + }); + } + + [Test] + public void LowerCaseTerm_IsCaseInsensitive() + { + Assert.That(Matches("strings", Element("App.Resources.Strings.Close")), Is.True); + } + + [Test] + public void Space_MeansAnd() + { + var element = Element("App.Resources.Strings.Close"); + + Assert.Multiple(() => + { + Assert.That(Matches("resources close", element), Is.True); + Assert.That(Matches("resources missing", element), Is.False); + }); + } + + [Test] + public void Pipe_MeansOr() + { + var element = Element("App.Views.MainWindow"); + + Assert.Multiple(() => + { + Assert.That(Matches("missing | views", element), Is.True); + Assert.That(Matches("missing | absent", element), Is.False); + }); + } + + [Test] + public void Or_BindsLessThanAnd() + { + // "views window | absent" reads as "(views AND window) OR absent". + Assert.Multiple(() => + { + Assert.That(Matches("views window | absent", Element("App.Views.MainWindow")), Is.True); + Assert.That(Matches("views absent | dialog", Element("App.Other.Dialog")), Is.True); + Assert.That(Matches("views absent | missing", Element("App.Views.MainWindow")), Is.False); + }); + } + + [Test] + public void MinusPrefix_ExcludesTheTerm() + { + Assert.Multiple(() => + { + Assert.That(Matches("-resources", Element("App.Resources.Strings.Close")), Is.False); + Assert.That(Matches("-resources", Element("App.Views.MainWindow")), Is.True); + }); + } + + [Test] + public void Negation_CombinesWithAPositiveTerm() + { + Assert.Multiple(() => + { + Assert.That(Matches("rules -tooltip", Element("App.Strings.Rules_Clear")), Is.True); + Assert.That(Matches("rules -tooltip", Element("App.Strings.Rules_Clear_Tooltip")), Is.False); + }); + } + + [Test] + public void SeveralNegations_ExcludeAll() + { + Assert.Multiple(() => + { + Assert.That(Matches("-tests -dsmsuite", Element("App.Views.MainWindow")), Is.True); + Assert.That(Matches("-tests -dsmsuite", Element("Tests.UnitTests.Foo")), Is.False); + Assert.That(Matches("-tests -dsmsuite", Element("DsmSuite.Viewer.Bar")), Is.False); + }); + } + + [Test] + public void Negation_BelongsToItsOwnOrGroup() + { + // "views -window | dialog" reads as "((NOT views) AND window) OR dialog" - the negation does not + // reach across the pipe. + Assert.That(Matches("views -window | dialog", Element("App.Other.Dialog")), Is.True); + } + + [Test] + public void Negation_AppliesToATypeTerm() + { + var property = Element("App.Strings.Close", type: CodeElementType.Property); + var method = Element("App.Service.Run", type: CodeElementType.Method); + + Assert.Multiple(() => + { + Assert.That(Matches("-type:property", property), Is.False); + Assert.That(Matches("-type:property", method), Is.True); + }); + } + + [Test] + public void Negation_AppliesToASourceTerm() + { + var external = Element("System.String", isExternal: true); + var internalElement = Element("App.Service"); + + Assert.Multiple(() => + { + Assert.That(Matches("-source:extern", external), Is.False); + Assert.That(Matches("-source:extern", internalElement), Is.True); + }); + } + + [Test] + public void Negation_AppliesToAPascalCaseTerm() + { + // An upper case letter switches the term to the ReSharper style regex ("DDG" -> DynamicDataGrid). + Assert.Multiple(() => + { + Assert.That(Matches("DDG", Element("App.Grids.DynamicDataGrid")), Is.True); + Assert.That(Matches("-DDG", Element("App.Grids.DynamicDataGrid")), Is.False); + Assert.That(Matches("-DDG", Element("App.Views.MainWindow")), Is.True); + }); + } + + [Test] + public void NegationDisabled_TreatsTheMinusAsPartOfTheTerm() + { + // The tree turns negation off: it expands and highlights every ancestor of a match, so an + // excluding search would unfold the whole tree. The '-' then searches literally and finds nothing + // instead of matching everything. + var element = Element("App.Resources.Strings.Close"); + + var withNegation = SearchExpressionFactory.CreateSearchExpression("-resources"); + var withoutNegation = SearchExpressionFactory.CreateSearchExpression("-resources", + SearchExpressionFactory.TextSearchField.FullName, false); + + Assert.Multiple(() => + { + Assert.That(withNegation.Evaluate(element), Is.False); + Assert.That(withoutNegation.Evaluate(element), Is.False); + Assert.That(withoutNegation.Evaluate(Element("App.Views.MainWindow")), Is.False); + }); + } + + [Test] + public void NegationDisabled_LeavesPositiveTermsUntouched() + { + var expression = SearchExpressionFactory.CreateSearchExpression("resources close", + SearchExpressionFactory.TextSearchField.FullName, false); + + Assert.That(expression.Evaluate(Element("App.Resources.Strings.Close")), Is.True); + } + + [Test] + public void LoneMinus_IsALiteralTerm() + { + // Nothing to negate. It must not become an empty term, which would match everything and turn the + // expression into "match nothing". + Assert.Multiple(() => + { + Assert.That(Matches("-", Element("App.My-Package.Widget")), Is.True); + Assert.That(Matches("-", Element("App.Views.MainWindow")), Is.False); + }); + } + + [Test] + public void MinusInsideATerm_IsNotANegation() + { + Assert.That(Matches("my-package", Element("App.My-Package.Widget")), Is.True); + } + + [Test] + public void NullElement_NeverMatches_NotEvenANegation() + { + // The tree has a virtual root without a code element. It answers "no" to every term, and an + // exclusion must not turn that into a match. + Assert.Multiple(() => + { + Assert.That(Matches("anything", null), Is.False); + Assert.That(Matches("-anything", null), Is.False); + Assert.That(Matches("-type:property", null), Is.False); + }); + } + + [Test] + public void NameField_SearchesTheNameInsteadOfTheFullName() + { + var element = Element("App.Resources.Strings", "Strings"); + + Assert.Multiple(() => + { + Assert.That(Matches("resources", element, SearchExpressionFactory.TextSearchField.Name), Is.False); + Assert.That(Matches("strings", element, SearchExpressionFactory.TextSearchField.Name), Is.True); + Assert.That(Matches("-resources", element, SearchExpressionFactory.TextSearchField.Name), Is.True); + }); + } + + [Test] + public void EmptySearch_MatchesNothing() + { + // Documents today's behaviour: an empty text yields no OR group at all, and an OR over nothing is + // false. Every caller short-circuits on empty input before building an expression, so this never + // shows up as "the filter hid everything". + Assert.That(Matches("", Element("App.Views.MainWindow")), Is.False); + } +} diff --git a/Tests/UnitTests/Xaml/XamlGraphLinkerTests.cs b/Tests/UnitTests/Xaml/XamlGraphLinkerTests.cs new file mode 100644 index 00000000..0064a388 --- /dev/null +++ b/Tests/UnitTests/Xaml/XamlGraphLinkerTests.cs @@ -0,0 +1,267 @@ +using CodeParserTests.Helper; +using CSharpCodeAnalyst.CodeGraph.Graph; +using CSharpCodeAnalyst.CodeParser.Xaml; + +namespace CodeParserTests.UnitTests.Xaml; + +[TestFixture] +public class XamlGraphLinkerTests +{ + [SetUp] + public void SetUp() + { + _graph = new TestCodeGraph(); + _directory = Path.Combine(Path.GetTempPath(), "XamlLinkerTests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(_directory); + } + + [TearDown] + public void TearDown() + { + if (Directory.Exists(_directory)) + { + Directory.Delete(_directory, true); + } + } + + private TestCodeGraph _graph = null!; + private string _directory = null!; + + /// + /// Builds "Assembly > Namespace > Type" the way the parser does: the namespace element carries + /// the whole dotted namespace, so the CLR name of the type is "App.Views.MyControl". The linker + /// resolves through the element names, so the ids stay short here. + /// + private CodeElement CreateType(CodeElement assembly, string namespaceName, string typeName) + { + var ns = assembly.Children.FirstOrDefault(c => c.Name == namespaceName) + ?? _graph.CreateNamespace(namespaceName, assembly); + return _graph.CreateClass(typeName, ns); + } + + private void WriteXaml(string relativePath, string content) + { + var path = Path.Combine(_directory, relativePath); + Directory.CreateDirectory(Path.GetDirectoryName(path)!); + File.WriteAllText(path, content); + } + + private static string[] EdgesFrom(CodeElement source, CodeGraph graph) + { + return source.Relationships + .Where(r => r.HasAttribute(RelationshipAttribute.IsXamlReference)) + .Select(r => graph.Nodes[r.TargetId].FullName) + .ToArray(); + } + + [Test] + public void Link_ElementTag_ConnectsCodeBehindToTheUsedType() + { + var assembly = _graph.CreateAssembly("App"); + var view = CreateType(assembly, "App.Views", "MainWindow"); + var control = CreateType(assembly, "App.Controls", "MyGrid"); + + WriteXaml("MainWindow.xaml", """ + + + + """); + + var added = XamlGraphLinker.Link(_graph, [new XamlProject(assembly, _directory)]); + + Assert.Multiple(() => + { + Assert.That(added, Is.EqualTo(1)); + Assert.That(EdgesFrom(view, _graph), Is.EqualTo(new[] { "MyGrid" })); + }); + } + + [Test] + public void Link_ObjectElement_AlsoConnectsToTheConstructor() + { + // Without this edge the constructor has no incoming reference at all, and everything only it + // calls dies with it once the analysis cascades. + var assembly = _graph.CreateAssembly("App"); + var view = CreateType(assembly, "App.Views", "MainWindow"); + var control = CreateType(assembly, "App.Controls", "MyGrid"); + var constructor = _graph.CreateMethod(".ctor", control); + + WriteXaml("MainWindow.xaml", """ + + + + """); + + XamlGraphLinker.Link(_graph, [new XamlProject(assembly, _directory)]); + + Assert.That(EdgesFrom(view, _graph), Is.EquivalentTo(new[] { "MyGrid", constructor.FullName })); + } + + [Test] + public void Link_XType_DoesNotConnectToTheConstructor() + { + // {x:Type} only names the type; nothing is created. + var assembly = _graph.CreateAssembly("App"); + var view = CreateType(assembly, "App.Views", "MainWindow"); + var control = CreateType(assembly, "App.Controls", "MyGrid"); + _graph.CreateMethod(".ctor", control); + + WriteXaml("MainWindow.xaml", """ + +