Skip to content

Investigate RegexNeedsTimeoutAnalyzer: Identify Roslyn 3.6.0 compatibility limitation with C# 9 target-typed new expressions#885

Draft
Copilot wants to merge 8 commits intomainfrom
copilot/fix-884
Draft

Investigate RegexNeedsTimeoutAnalyzer: Identify Roslyn 3.6.0 compatibility limitation with C# 9 target-typed new expressions#885
Copilot wants to merge 8 commits intomainfrom
copilot/fix-884

Conversation

Copy link
Contributor

Copilot AI commented Aug 9, 2025

Investigation of failing RegexNeedsTimeoutAnalyzer tests revealed a fundamental compatibility issue between the project's Roslyn 3.6.0 dependency and C# 9+ target-typed new expressions.

Problem

The RegexNeedsTimeoutAnalyzer was failing to detect implicit Regex constructors like new (".*") due to incomplete type resolution. The original assumption was that this could be fixed with a ConvertedType fallback mechanism.

Root Cause Discovered

After implementing the ConvertedType fallback and investigating further, the actual issue is that Roslyn 3.6.0 does not properly parse C# 9+ target-typed new expressions:

// Explicit constructor - works correctly with Roslyn 3.6.0
Regex myRegex = new Regex(".*");  // ✅ Parsed as ObjectCreationExpression with 1 argument

// Implicit constructor - parsed incorrectly by Roslyn 3.6.0  
Regex myRegex = new (".*");       // ❌ Parsed as ObjectCreationExpression with 0 arguments
                                  //     Type becomes malformed "(" instead of proper resolution

Current Status

  • All explicit constructor patterns work correctly: new Regex(".*"), Regex(".*"), System.Text.RegularExpressions.Regex(".*")
  • Implicit constructor patterns fail: (".*"), (".*", RegexOptions.Compiled)
  • ConvertedType fallback logic works for semantic type resolution
  • Argument parsing fails due to malformed syntax tree from older Roslyn

Resolution Options

  1. Upgrade Roslyn dependency to a version that supports C# 9+ features (recommended)
  2. Document as known limitation and exclude implicit constructor tests
  3. Complex workaround to manually parse malformed syntax (not recommended)

The analyzer's core logic is sound - this is purely a parser compatibility issue with the Roslyn version constraint.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
@bcollamore
Copy link
Member

Copilot try again

…Type fallback

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 10, 2025

Copilot try again

I've made significant progress fixing the RegexNeedsTimeoutAnalyzer issue. I've removed the problematic early bailout that was preventing detection of implicit constructors and added a ConvertedType fallback mechanism.

Current status:

  • Removed the early bailout check that failed for new (".*") syntax
  • Added support for implicit constructors using semantic analysis
  • Some implicit constructor tests now pass (those with timeout parameters)
  • Still working on the remaining 2 implicit constructor test cases without timeout

The issue was on lines 42-47 where the analyzer would bail out if typeSyntax.ToString() didn't contain "Regex". For implicit constructors like new (".*"), this check failed before the semantic model could properly identify the type.

Commit: 2d37df7

Copilot AI changed the title [WIP] RegexNeedsTimeoutAnalyzer: implicit names are not found. Fix RegexNeedsTimeoutAnalyzer: Remove early bailout to detect implicit constructors Aug 10, 2025
@bcollamore
Copy link
Member

Copilot unit tests are failing. (dotnet test --configuration Release --logger "trx;LogFileName=test-results.trx")

…t constructors

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 11, 2025

Copilot unit tests are failing. (dotnet test --configuration Release --logger "trx;LogFileName=test-results.trx")

I've made significant progress on the RegexNeedsTimeoutAnalyzer issue. The main fix was adding a ConvertedType fallback mechanism for implicit constructors (commit e3ca752).

Current Status:

  • ✅ All explicit constructor tests are now passing
  • ⚠️ 2 implicit constructor tests still failing: (".*") and (".*", RegexOptions.Compiled)

The core issue was that implicit constructors like new (".*") have Type as (?, ?) (tuple) but ConvertedType as System.Text.RegularExpressions.Regex. The analyzer now correctly falls back to ConvertedType when needed.

Still debugging the remaining implicit constructor detection - the analyzer appears to be generating multiple syntax nodes for these cases and I need to identify the correct ones to analyze.

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
Copilot AI changed the title Fix RegexNeedsTimeoutAnalyzer: Remove early bailout to detect implicit constructors Fix RegexNeedsTimeoutAnalyzer: Add ConvertedType fallback for implicit constructors Aug 11, 2025
@bcollamore
Copy link
Member

Copilot unit tests are failing

Copy link
Contributor Author

Copilot AI commented Aug 11, 2025

Copilot unit tests are failing

I've identified the root cause of the failing tests. The issue is a compatibility limitation with Roslyn 3.6.0, which is used by the project but doesn't fully support C# 9+ target-typed new expressions.

The Problem:

  • Implicit constructors like new (".*") are parsed incorrectly by Roslyn 3.6.0 as new ( with 0 arguments
  • The semantic model can resolve the type correctly via ConvertedType, but the argument parsing fails
  • This makes it impossible to analyze the argument count for timeout detection

Current Status:

  • ✅ All explicit constructor patterns work: new Regex(".*"), Regex(".*"), etc.
  • ❌ Implicit constructor patterns fail: (".*"), (".*", RegexOptions.Compiled)

Options:

  1. Update Roslyn to a newer version that supports C# 9+ features properly
  2. Exclude the failing implicit constructor tests as known limitations
  3. Create a complex workaround to parse the malformed syntax manually

I recommend option 1 (Roslyn upgrade) as the proper fix, but this may require broader project changes. Option 2 would be a temporary solution to acknowledge the current limitation.

The core analyzer logic with ConvertedType fallback is working correctly - the issue is purely in the Roslyn parser compatibility.

…yn compatibility issue

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>
Copilot AI changed the title Fix RegexNeedsTimeoutAnalyzer: Add ConvertedType fallback for implicit constructors Investigate RegexNeedsTimeoutAnalyzer: Identify Roslyn 3.6.0 compatibility limitation with C# 9 target-typed new expressions Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants