Skip to content

fix(cli): narrow parse_subnet_option/1 spec to the value it returns #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
toshi0806 merged 2 commits into from
Aug 11, 2026
Merged

fix(cli): narrow parse_subnet_option/1 spec to the value it returns #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2bc0f3
fix(cli): narrow parse_subnet_option/1 spec to the value it returns
toshi0806 Aug 11, 2026
3466955
fix: anchor the escript ignore pattern to the repository root
toshi0806 Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ tdig-*.tar
/tmp/

mix.lock
tdig
# The escript binary, at the repository root only. Unanchored, this pattern
# also matches the lib/tdig/ directory, and a file added there is ignored
# without ever showing up in `git status`.
/tdig

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✨ [POSITIVE] tdig から /tdig への修正は、lib/tdig/ ディレクトリ配下の新規ファイルが暗黙に無視される問題を的確に解消しており、コメントで理由も明記されている点が非常に良いです。

test_release/

# Dialyzer PLT files (contain absolute paths from local build env; regenerated by CI)
Expand Down
19 changes: 18 additions & 1 deletion lib/tdig/cli.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,24 @@ defmodule Tdig.CLI do
|> Map.put(:options, [ecs_option])
end

@spec parse_subnet_option(String.t()) :: {atom(), map()}
@typedoc """
An EDNS Client Subnet option, shaped as `Tenbin.DNS` expects in `:options`.

`family` is 1 for IPv4 and 2 for IPv6 (RFC 7871). `source_prefix` is the
prefix length given on the command line, capped at the family's width;
`scope_prefix` is always 0 in a query, and only a response carries a
meaningful value.
"""
@type edns_client_subnet ::
{:edns_client_subnet,
%{
family: 1 | 2,
client_subnet: :inet.ip_address(),
source_prefix: integer(),

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ [LOW] source_prefix: integer() について確認です。RFC 7871 のプレフィックス長は 0〜128 の非負値であり、実際に負値になることはないはずです。より success typing に近づけるなら non_neg_integer()（あるいは実装が返し得る具体的な範囲）にした方が、:underspecs 観点でも意図が明確になります。実装が本当に任意の integer を返し得るのであれば現状で問題ありませんが、範囲を絞れるなら検討をおすすめします。

@toshi0806 toshi0806 Aug 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

「実装が本当に任意の integer を返し得るのであれば現状で問題ありません」という条件のほうが成立していました。実測です。

192.0.2.1/24:   source_prefix: 24
192.0.2.1/-5:   source_prefix: -5     ← 負値が通る
192.0.2.1/999:  source_prefix: 32     ← 上限だけは効く
2001:db8::1/-1: source_prefix: -1

prefix = String.to_integer(prefix_str) は "-5" を素直に -5 にし、min(prefix, 32) は上限しか抑えないためです。RFC 7871 上は 0〜128 ですが、この実装はその範囲を検証していません。

したがって non_neg_integer() にすると spec が実装より狭くなり、実際に返る値を排除する嘘の契約になります（Dialyzer の success typing も integer() です）。ここは integer() のまま据え置きます。

ただし指摘の背後にある「負の prefix は本来受け付けるべきでない」という点は妥当で、これは spec ではなく入力検証の欠落です。値域外を弾くのかクランプするのかは挙動の変更を伴うため、本 PR（Dialyzer の解消）とは分けて #86 に起票しました。

scope_prefix: 0
}}

@spec parse_subnet_option(String.t()) :: edns_client_subnet()
def parse_subnet_option(subnet) do
case String.split(subnet, "/") do
[addr_str, prefix_str] ->
Expand Down
Loading