Skip to content

Fix #16268: classify IDisposable in type-occurrence positions as Interface, not DisposableType#19809

Open
T-Gro wants to merge 4 commits into
mainfrom
fix/issue-16268
Open

Fix #16268: classify IDisposable in type-occurrence positions as Interface, not DisposableType#19809
T-Gro wants to merge 4 commits into
mainfrom
fix/issue-16268

Conversation

@T-Gro
Copy link
Copy Markdown
Member

@T-Gro T-Gro commented May 26, 2026

Description

Fixes #16268

When a type like IDisposable appears in a type-occurrence position (e.g., type annotations, interface implementations), it was being classified as DisposableType instead of Interface. This caused incorrect semantic highlighting in editors.

Changes

  • src/Compiler/Service/SemanticClassification.fs: In the type-occurrence classification logic, check isInterfaceTy before isDisposableTy. If the type is an interface (like IDisposable), classify it as Interface rather than falling through to the DisposableType check.
  • ** ests/FSharp.Compiler.ComponentTests/Language/SemanticClassificationRegressions.fs**: Added regression tests verifying that IDisposable and other interface types in type annotations are correctly classified as Interface.

@T-Gro T-Gro requested a review from a team as a code owner May 26, 2026 09:14
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026 •
edited
Loading

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label May 26, 2026
T-Gro
T-Gro commented May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member Author

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Looks good — clean, correct fix with solid test coverage.

The fix: Reordering isInterfaceTy before isDisposableTy is the right approach. Interface types (including IDisposable itself) should always be classified as Interface in semantic classification — the DisposableType classification is meant for concrete classes that implement IDisposable, not for interface types that happen to extend it.

Tests: Good coverage across four distinct scenarios: interface impl position, concrete disposable class (negative test guarding against regression), type constraint usage, and a custom non-IDisposable interface.

Minor nit (non-blocking): The test file is missing a trailing newline (the diff shows \ No newline at end of file).

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label May 26, 2026
@T-Gro T-Gro requested a review from abonie May 28, 2026 14:32
@T-Gro T-Gro enabled auto-merge (squash) May 28, 2026 14:33
@T-Gro
Copy link
Copy Markdown
Member Author

T-Gro commented May 28, 2026

@copilot : Pls add release notes

auto-merge was automatically disabled May 28, 2026 14:44

Head branch was pushed to by a user without write access

Copy link
Copy Markdown
Contributor

Copilot AI commented May 28, 2026

@copilot : Pls add release notes

Added in commit Add release notes for #16268 fix and add trailing newline to test file. Also fixed the missing trailing newline in the test file.

Copilot finished work on behalf of T-Gro May 28, 2026 14:44
T-Gro and others added 4 commits May 30, 2026 00:24
@T-Gro
Fix #16268: classify IDisposable in type-occurrence positions as Inte…
9743552 
…rface, not DisposableType

Reorder the classification checks in SemanticClassification.fs so that
isInterfaceTy is checked before isDisposableTy. This ensures that interface
types (including System.IDisposable itself) used in type-occurrence positions
(e.g. `interface IDisposable with`, `#IDisposable` constraints, upcasts)
are colorized as Interface rather than DisposableType. Concrete disposable
classes such as MemoryStream remain classified as DisposableType via the
unchanged Item.CtorGroup arm.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Apply remaining changes
82355d1
@T-Gro
Remove executor artifact
c8f61ff
@T-Gro
Add release notes for #16268 fix and add trailing newline to test file
298dba9 
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
@T-Gro T-Gro force-pushed the fix/issue-16268 branch from fc41adc to 298dba9 Compare May 29, 2026 22:39
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonie abonie Awaiting requested review from abonie

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

F# Compiler and Tooling
Status: New

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent coloring of interface inheriting IDisposable

2 participants

@T-Gro