feat(rules): add F# review support by yaodong-shen · Pull Request #1448 · alibaba/open-code-review

yaodong-shen · 2026-09-19T04:54:18Z

Description

Adds focused review coverage for F# source files.

maps .fs, .fsi, and .fsx files to a conservative F# system rule;
adds .fsi and .fsx to the review allowlist;
covers case-insensitive allowlist handling and rule resolution for all three F# extensions.

The rule prioritizes concrete correctness, security, lifetime, async/cancellation, sequence, interop, and module-boundary defects; it explicitly avoids formatting and unsupported style preferences.

Closes #1447

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring (no functional changes)
Documentation update
CI / Build / Tooling

How Has This Been Tested?

make test passes locally
make check passes locally
make build passes locally
make coverage passes locally (92.5%, above the 90% threshold)
ocr review --audience agent --preview inspected the changed configuration scope; a full LLM review cannot run locally because no OCR provider is configured.

Checklist

My code follows the project coding style (make check)
I have performed a self-review of my code
I have added tests that prove the extension and rule mapping work
New and existing unit tests pass locally with my changes
I have updated the documentation accordingly
I have signed the CLA
I used AI/LLM and disclose it below; I reviewed the output and will respond to maintainer questions from my own understanding.

Codex (GPT-5) was used for development assistance. I reviewed the resulting change and did not add AI attribution trailers to the commit.

Related Issues

Closes #1447

CLAassistant · 2026-09-19T04:54:26Z

All committers have signed the CLA.

github-actions · 2026-09-19T04:54:43Z

✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

wu21-web · 2026-09-19T05:04:32Z

+- `match` expressions that omit a reachable discriminated-union or `option` case, especially after a union gains a new case; do not report a match that the compiler can prove exhaustive
+- Catch-all `_` branches used only to suppress an incomplete-pattern warning when an omitted case needs distinct behavior or error handling
+- Active patterns or guards whose ordering shadows a later reachable case, silently selecting the wrong branch
+- `Option.get`, `Option.Value`, `Result.get`, or equivalent unwraps where `None` or `Error` can occur for runtime, external, or untrusted input


Option.get exists, but the FSharp.Core Result module does not define Result.get.

Suggested change

- `Option.get`, `Option.Value`, `Result.get`, or equivalent unwraps where `None` or `Error` can occur for runtime, external, or untrusted input

- `Option.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input

Applied in 6898498. I verified that Result.get is not part of FSharp.Core, so the rule now names Option getters and project-specific partial Result unwraps.

wu21-web · 2026-09-19T05:16:30Z

+#### .NET Interop and Type Boundaries
+- Passing F# `option` values through a .NET API as though they were `null`, or treating a nullable/reference return from .NET as non-null without a local invariant
+- P/Invoke, reflection, serialization, or JSON bindings whose declared types, field names, nullability, ownership, or enum values do not match the external contract
+- Unsafe casts (`unbox`, `:?`, `Unchecked.defaultof`, or reflection-based invocation) without a locally established runtime type/nullability invariant


:? is a type test, not an unsafe cast

Suggested change

- Unsafe casts (`unbox`, `:?`, `Unchecked.defaultof`, or reflection-based invocation) without a locally established runtime type/nullability invariant

- Runtime downcasts or unchecked/default-producing operations (`:?>`, `unbox`, `Unchecked.defaultof`, or reflection-based invocation) without a locally established runtime type/nullability invariant

Applied in 6898498. :? is a type test; the rule now targets runtime downcasts (:?>) and other unchecked/default-producing operations.

wu21-web · 2026-09-19T05:17:17Z

+- Untrusted values flowing into SQL, shell commands, file paths, URLs, deserialization, or HTML without validation or parameterization, and secrets written to source, logs, or error messages
+
+#### Signatures and Module Boundaries
+- An `.fsi` signature that promises a different type, exception behavior, mutability contract, or visibility from the implementation it exposes


Suggested change

- An `.fsi` signature that promises a different type, exception behavior, mutability contract, or visibility from the implementation it exposes

- An `.fsi` signature whose exposed types, arity, generic constraints, mutability, or visibility do not match the implementation or intended public API

Applied in 6898498. The signature guidance now covers exposed types, arity, generic constraints, mutability, and visibility against the implementation or intended public API.

NanaseInori

The follow-up commit addresses the F# accuracy issues raised in the earlier review. The rule now uses the correct FSharp.Core terminology (Option access / project-specific partial Result unwraps and :?> for runtime downcasts), and the .fsi guidance is phrased around the actual signature contract.

The extension allowlist and default-rule resolution cover .fs, .fsi, and .fsx, including case-insensitive allowlist tests. I don't see any remaining blocking issue in the current head.

This review was conducted by NanaseInori's review bot, using the model GPT-5.6 Sol. If you need a human review, please manually @NanaseInori.

wu21-web

Please update the site documentation.

yaodong-shen · 2026-09-19T10:55:23Z

Updated in 9863efa. The site Review Rules page now lists the built-in **/*.{fs,fsi,fsx} -> fsharp.md mapping in the English, Chinese, Japanese, Korean, and Russian docs, including its implementation/signature/script scope.

git diff --check, make check, race-enabled make test, and make build pass locally. The Pages dependency tree is not installed in this checkout, so its npm test/build gates are unrun rather than reported as passing.

lizhengfeng101

LGTM

wu21-web

Add an exclude pattern, for example a glob like, **/*.Test.fs or**/*Test.fs, and add it to the test index.

wu21-web · 2026-09-19T14:31:15Z

+- `match` expressions that omit a reachable discriminated-union or `option` case, especially after a union gains a new case; do not report a match that the compiler can prove exhaustive
+- Catch-all `_` branches used only to suppress an incomplete-pattern warning when an omitted case needs distinct behavior or error handling
+- Active patterns or guards whose ordering shadows a later reachable case, silently selecting the wrong branch
+- `Option.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input


You missed one

Suggested change

- `Option.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input

- `Option.get`, `ValueOption.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input

Implemented in a8a0488. ValueOption.get is now listed alongside Option.get; make check, make test, make coverage, and make build all pass.

yaodong-shen · 2026-09-19T15:42:59Z

Updated in 2abe064. Added **/*Test.fs to the default exclude patterns and covered nested and root F# test-file matches plus a TestSupport.fs non-match in the exclusion test index. The default-path exclusion list in all five site locales now includes the same pattern.

git diff --check, make check, race-enabled make test, make coverage, and make build pass locally.

yaodong-shen · 2026-09-19T15:44:50Z

Follow-up: upstream main advanced after the previous push, so I rebased the focused branch onto a003b93 and force-pushed with an exact lease. The current head is 8364f31; there were no conflicts, and git diff --check, make check, race-enabled make test, make coverage, and make build were rerun successfully.

Qiyuanqiii

good

lizhengfeng101 · 2026-09-21T07:25:18Z

@yaodong-shen please rebase main

# Conflicts: # pages/src/content/docs/en/review-rules.md # pages/src/content/docs/ja/review-rules.md # pages/src/content/docs/ko/review-rules.md # pages/src/content/docs/ru/review-rules.md # pages/src/content/docs/zh/review-rules.md

wu21-web

LGTM
Awaiting CI

lizhengfeng101

LGTM

NanaseInori

LGTM

wu21-web suggested changes Sep 19, 2026

View reviewed changes

NanaseInori approved these changes Sep 19, 2026

View reviewed changes

wu21-web suggested changes Sep 19, 2026

View reviewed changes

lizhengfeng101 reviewed Sep 19, 2026

View reviewed changes

Comment thread internal/config/rules/rule_docs/fsharp.md Outdated

lizhengfeng101 previously approved these changes Sep 19, 2026

View reviewed changes

wu21-web suggested changes Sep 19, 2026

View reviewed changes

yaodong-shen dismissed lizhengfeng101’s stale review via 2abe064 September 19, 2026 15:42

yaodong-shen and others added 5 commits September 19, 2026 23:43

feat(rules): add F# review support

034c6ae

fix(rules): correct F# review guidance

f11c5fa

docs(site): list the F# review rule

979b829

feat: Apply suggestion from @lizhengfeng101

0f53eae

fix(allowlist): exclude F# test files

8364f31

yaodong-shen force-pushed the feat/fsharp-review-rules branch from 2abe064 to 8364f31 Compare September 19, 2026 15:44

docs(rules): cover ValueOption.get

a8a0488

wu21-web approved these changes Sep 19, 2026

View reviewed changes

Qiyuanqiii approved these changes Sep 19, 2026

View reviewed changes

wu21-web approved these changes Sep 21, 2026

View reviewed changes

lizhengfeng101 approved these changes Sep 21, 2026

View reviewed changes

NanaseInori approved these changes Sep 21, 2026

View reviewed changes

lizhengfeng101 merged commit 1a33a98 into alibaba:main Sep 21, 2026
15 checks passed

	- `Option.get`, `Option.Value`, `Result.get`, or equivalent unwraps where `None` or `Error` can occur for runtime, external, or untrusted input
	- `Option.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input

	- Unsafe casts (`unbox`, `:?`, `Unchecked.defaultof`, or reflection-based invocation) without a locally established runtime type/nullability invariant
	- Runtime downcasts or unchecked/default-producing operations (`:?>`, `unbox`, `Unchecked.defaultof`, or reflection-based invocation) without a locally established runtime type/nullability invariant

	- An `.fsi` signature that promises a different type, exception behavior, mutability contract, or visibility from the implementation it exposes
	- An `.fsi` signature whose exposed types, arity, generic constraints, mutability, or visibility do not match the implementation or intended public API

	- `Option.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input
	- `Option.get`, `ValueOption.get`, accessing `.Value` on an option/value option, or project-specific partial `Result` unwraps where `None`, `ValueNone`, or `Error` can occur for runtime, external, or untrusted input

Conversation

yaodong-shen commented Sep 19, 2026

Description

Type of Change

How Has This Been Tested?

Checklist

Related Issues

Uh oh!

CLAassistant commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NanaseInori left a comment

Choose a reason for hiding this comment

Uh oh!

wu21-web left a comment

Choose a reason for hiding this comment

Uh oh!

yaodong-shen commented Sep 19, 2026

Uh oh!

Uh oh!

lizhengfeng101 left a comment

Choose a reason for hiding this comment

Uh oh!

wu21-web left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaodong-shen commented Sep 19, 2026

Uh oh!

yaodong-shen commented Sep 19, 2026

Uh oh!

Qiyuanqiii left a comment

Choose a reason for hiding this comment

Uh oh!

lizhengfeng101 commented Sep 21, 2026

Uh oh!

wu21-web left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizhengfeng101 left a comment

Choose a reason for hiding this comment

Uh oh!

NanaseInori left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CLAassistant commented Sep 19, 2026 •

edited

Loading

wu21-web left a comment •

edited

Loading