[cgarling]

Fixes #41, #247.

tag:yaml.org,2002:int's implicit resolver regex (resolver.jl) tags any all-digit scalar as int, leading zeros included. construct_yaml_int then treats any int-tagged value starting with 0 as octal and calls parse(Int, value; base=8) without checking the digits are actually 0-7, so e.g. "08" or a zero-padded ID string throws ArgumentError instead of loading. Tightened the decimal branch to exclude spurious leading zeros (0, or [1-9][0-9]*) and the octal branch to the bare-leading-zero form YAML 1.1 actually specifies (0[0-7]+ -- it previously required a 0o prefix, unused by any existing test). Applied the same fix to the adjacent float branch, since its bare-digit alternative has the same gap and would otherwise silently mis-tag these values as floats once disqualified from int.

#45 patched this at the construct_yaml_int call site with a try/catch; per review discussion there this instead fixes it at the resolver, so an ambiguous scalar is never tagged int to begin with. Regression test added; full suite passes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.70%. Comparing base (7fb9a61) to head (2065f54).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
+ Coverage   87.59%   87.70%   +0.11%     
==========================================
  Files          15       15              
  Lines        1652     1667      +15     
==========================================
+ Hits         1447     1462      +15     
  Misses        205      205              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kescobo]

From your tests, the fact that 014 gets parsed as octal but 089 as a string is weird to me, but clearly it's what you expected. Is this behavior spelled out in a yaml spec somewhere (also relevant maybe: #133)?

[cgarling]

I was looking at the 1.1 spec https://yaml.org/type/int.html see the resolution and validation section.

I think I mixed up the YAML version specs (sorry, I'm not an expert here).

It looks to me like the int resolver regex on master is YAML 1.2 spec (uses |0o [0-7]+ ) but construct_yaml_int is 1.1 spec because values with a leading 0 get parsed as base-8 before they reach the else statement that parses base-10 (src/constructor.jl:269). For the 1.2 spec, the base-8 parser should require leading 0o. Note that "0x" leading goes to base-16, plain "0" stays base-10 due to length(value) > 1 guard on line 269, and signed values like -014 start with -, so they skip the leading-zero check and go to base-10.

YAML.jl/src/constructor.jl

Lines 255 to 274 in 68f685c

	function construct_yaml_int(constructor::Constructor, node::Node)
	value = string(construct_scalar(constructor, node))
	value = lowercase(replace(value, "_" => ""))
	if in(':', value)
	# TODO
	#throw(ConstructorError(nothing, nothing,
	#"sexagesimal integers not yet implemented", node.start_mark))
	@warn "sexagesimal integers not yet implemented. Returning String."
	return value
	end
	if length(value) > 2 && value[1] == '0' && (value[2] == 'x' || value[2] == 'X')
	return parse(Int, value[3:end], base = 16)
	elseif length(value) > 1 && value[1] == '0'
	return parse(Int, value, base = 8)
	else
	return parse(Int, value, base = 10)
	end
	end

So I think what's happening is the 1.2-spec resolver tags the scalar as int, then the 1.1 constructor tries to parse it as octal and throws.

I think a better fix, now that I understand the code better, would be to keep the resolver as-is and only change construct_yaml_int to only parse scalars as octals if they have leading "0o". I think "0b" prefixes for base 2 also get parsed as base 8 currently and need the same fix if you want to support them. This would be breaking for unsigned leading-zero integers made only of the digits 0-7, which currently load as octal but would load as decimal (e.g. 014 would change from 12 to 14, and 0755 from 493 to 755), so the spec-02-19.yaml fixture would need updating too. I can make that change if you prefer

[kescobo]

That makes sense to me, but I'd love another set of eyes on it - I'll post this issue to the data channel in slack and see if there are any bites.

I was looking at the 1.1 spec https://yaml.org/type/int.html see the resolution and validation section.

I concur with your conclusion here - they explicitly have 02472256 being octal. There are only two mentions of octal in the 1.2 spec (and not under integers), both of which us the 0o notation. But under tag resolution, that notation is explicitly marked as invalid... I'm not sure what that means.

[quinnj]

Reviewed head 04bbff981d5a608226391eb119055fb7e873a093 against base 68f685c61f9b63af593ee3ca46d45007f42414b2. A focused check for eight leading-zero fractional/exponent scalars passes all 16 assertions on the base and fails all 16 on this head: valid numbers now become strings. This needs to be corrected before merge.

For the earlier schema question: 0o7 is marked invalid in the YAML 1.2 JSON-schema example; the separate Core schema resolution table explicitly accepts 0o octal notation. The existing resolver and constructor disagree, so the desired integer policy still needs to be settled. The float regression below can be addressed independently.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

[quinnj]

[P2] Preserve leading-zero fractions and exponent forms

The new mantissa restriction also rejects actual floating-point notation. YAML.load("01.5") changes from 1.5 on the base to "01.5" on this head; "01e2", "001.5", "+01.5", and "-01.5" likewise become strings. These forms are accepted by the YAML 1.2 Core float pattern and by the existing package. Keep leading zeros when a decimal point or exponent identifies a float; restrict only the bare-digit fallback needed for the identifier fix. Add regression checks for both fraction and exponent forms.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

[cgarling]

Thanks for the review @quinnj, I revised the regex and added regression tests.

[quinnj-bot]

The earlier leading-zero float regression is fixed. Reviewed head c832b70f0226cd97faa0a98546969c951a0b8a4c against base/merge-base 7fb9a6105c0d15c1f10be1131e424231ab90aa66; one remaining compatibility regression is noted inline.

Validation on Julia 1.12.7: full Pkg.test("YAML") passed, plus 6,530 focused head assertions covering integers, floats, prefixes, quoted/explicit tags, mappings, sequences, and file/document entry points. The earlier eight float cases pass all 16 assertions on both base and head. All 22 CI checks are green at this exact head, including Julia 1.0; the local Julia 1.0 binary cannot run on this Mac.

The additional custom-integer regression passes 6/6 assertions on the base and fails 6/6 on this head. Restoring only the removed 0o resolver alternative in the test process passes those six checks and nine checks preserving the leading-zero fix. This correction can preserve existing behavior without settling broader YAML schema policy.

AI disclosure: This review was prepared with OpenAI Codex.

[quinnj-bot]

[P2] Retain 0o resolution for custom integer constructors

Removing the existing 0o[0-7]+ alternative also breaks successful loads through the supported constructor override:

ints = Dict{String,Function}("tag:yaml.org,2002:int" =>
    (c, n) -> parse(BigInt, YAML.construct_scalar(c, n)))
YAML.load("0o14", ints)

This returns BigInt(12) on the base but the string "0o14" here: the custom integer constructor is no longer called. Long octal values have the same regression. The default constructor's pre-existing 0o error does not cover this working custom-constructor path. Keep the old |0o [0-7]+ alternative alongside the new bare-leading-zero alternative, and add a constructor-override regression test.

[cgarling]

Thanks, I put the 0o[0-7]+ back and added a regression test

[quinnj-bot]

[review generated via AI]

Re-reviewed head 2065f54 against 7fb9a61. All three earlier findings are resolved, and I'd merge this as is.

I loaded 39 scalars on base and head, with the default constructors and with a custom base-10 BigInt int constructor (parse(BigInt, YAML.construct_scalar(c, n))):

• Only the four scalars from #41 and #247 change. "08", "089", "019" and "0123456789" threw ArgumentError on base and load as strings here.
• Leading-zero floats are unchanged: 01.5, 001.5, +01.5, -01.5, 01e2, 089.5 and 08e1 give the same Float64 on both.
• 0o14 still reaches a custom int constructor (12::BigInt on both).
• Everything else matches base: 0, 00, 007, 014, +08, -012, 0x1F, the signed binary forms, 1e5, .5, 1., .inf, .nan, 1_000 and 1:30.
• All 22 CI checks pass at this head.

Two notes, neither blocking:

1. A custom int constructor no longer receives unsigned leading-zero scalars that contain an 8 or 9. YAML.load("08", ints) gave 8::BigInt on base and gives "08" here. That matches YAML 1.1 and PyYAML, which this package's octal handling already follows, but it's worth a line in the release notes.
2. A separate, existing bug, not from this PR: with the default constructors, YAML.load("0o14") throws on both base and head. The resolver tags 0o14 as an int, then construct_yaml_int passes it to parse(Int, "0o14"; base = 8), which rejects the o. A small follow-up could strip the 0o prefix there.