Skip to content

phase1: F-CP - pin structural CurvePolygon contract with a spec test - #9

Open
grootstebozewolf wants to merge 12 commits into
NetTopologySuite:developfrom
grootstebozewolf:phase1/f-cp-spec
Open

phase1: F-CP - pin structural CurvePolygon contract with a spec test#9
grootstebozewolf wants to merge 12 commits into
NetTopologySuite:developfrom
grootstebozewolf:phase1/f-cp-spec

Conversation

@grootstebozewolf

Copy link
Copy Markdown

Summary

Phase 1 / F-CP of the SFA Curve Awareness epic (locationtech/jts#1195) — Structural CurvePolygon. The gating sub-issue for every Phase-2 TAG that needs to talk about a curved boundary, area, or validity.

Stacks on top of #8 (rename + analyzer) which stacks on #7 (CI wake-up). Merge order: #7#8 → this.

The headline

F-CP needs no implementation work in C#. All ten sub-TAG assertions pass against enhancement/curved HEAD out of the box. This PR is the regression net that pins that, plus the doc explaining why.

The architectural background

On the JTS side, F-CP is a real implementation problem because Polygon.getExteriorRing() is typed LinearRing. The epic's §7 risk #1 (FCP-DOVE) tracks the option-A/B/C dovetail choice for the legacy accessor — see SPEC_F_CP.md and the F-CP option spikes.

On the C# side, the dovetail does not exist. NetTopologySuite.Curve's CurvePolygon extends Surface<Curve> (FObermaier's generic abstract base on NTS PR #526), not Polygon:

public class CurvePolygon : Surface<Curve>, ILinearizable<Polygon>
{
    public override Curve ExteriorRing { get; }
    public override Curve GetInteriorRingN(int index) { ... }
}

The shell and holes are typed Curve — the polymorphic base of LinearRing / LineString / CircularString / CompoundCurve. This is, in effect, the endpoint Option-B-on-Polygon would have reached if the legacy LinearRing accessor had never existed. So it never has to be widened, and there is no blast-radius count to do.

Sub-TAG mapping

Sub-TAG C# status Pinned by
FCP-S green by architecture FCP_S_compound_shell_…, FCP_S_arc_shell_…
FCP-MEM green (verified) FCP_MEM_compound_shell_members_retain_subtypes
FCP-H green by architecture FCP_H_arc_hole_exposed_as_CircularString
FCP-CP green by architecture FCP_CP_copy_preserves_shell_subtype, …_arc_hole_subtype
FCP-TL green by architecture FCP_TL_linearisation_walks_shell_and_holes
FCP-WKT green (verified) FCP_WKT_roundtrip_preserves_compound_shell, …_arc_shell
FCP-DOVE not applicable FCP_DOVE_not_applicable_in_csharp — reflection-asserts ExteriorRing returns Curve, not LinearRing. If a future refactor reintroduces a LinearRing-typed accessor, this test goes red and the epic §7 risk #1 conversation has to happen for this repo too.

What this PR adds

  • docs/SPEC_F_CP.md — the C# companion to JTS' SPEC_F_CP.md. Sub-TAG-by-sub-TAG mapping, architectural justification for skipping FCP-DOVE, cross-reference pointers into NetTopologySuite.Proofs for the Phase-2 TAGs that will lean on the proofs explicitly.
  • test/.../CurveAwareness/CurvePolygonStructuralSpec.cs — ten test methods covering the six functional sub-TAGs + the meta-test for FCP-DOVE. NTS convention (assert-style regression net, not the JTS "delete on green") — the class stays in the codebase indefinitely as a regression net.

Empirical result

Passed FCP_S_compound_shell_exposed_as_CompoundCurve
Passed FCP_S_arc_shell_exposed_as_CircularString
Passed FCP_MEM_compound_shell_members_retain_subtypes
Passed FCP_H_arc_hole_exposed_as_CircularString
Passed FCP_CP_copy_preserves_shell_subtype
Passed FCP_CP_copy_preserves_arc_hole_subtype
Passed FCP_TL_linearisation_walks_shell_and_holes
Passed FCP_WKT_roundtrip_preserves_arc_shell
Passed FCP_WKT_roundtrip_preserves_compound_shell
Passed FCP_DOVE_not_applicable_in_csharp

Test Run Successful.  Total tests: 10

WKTReaderEx already preserves COMPOUNDCURVE member subtypes on parse; WKTWriterEx already emits the structural tags. Hat tip to FObermaier's original 2021 design.

What's NOT in this PR

  • Any implementation change to CurvePolygon.cs or the WKT/WKB IO. This PR is documentation + assertions.
  • F-MC / F-MS / F-RD (the other Phase-1 sub-TAGs). F-MC and F-MS are reportedly half-already-free on the JTS Phase-1 base per JTS #1195 comment 4436748851; a follow-up PR will run the same SPEC + test pattern for both.
  • Phase 2+ TAGs (M-LEN-CS, M-AREA-CP, V-CP, B-CP, …). They depend on F-CP landing, and now it has.

Test plan

AI assistance disclosure

AI-drafted, human-reviewed and curated. AI-generated portions are dedicated to CC0-1.0; human curation falls under the NTS BSD-3-Clause grant.

SPDX-License-Identifier: BSD-3-Clause AND CC0-1.0
Assisted-by: Claude (Opus-4.7)

🤖 Generated with Claude Code

FObermaier and others added 12 commits June 10, 2021 14:25
Improve flattening quality for ArcSegmentLength = 0d
Add intitial tests for CurveGeometryFactoryTest
Move functionality to Curve and Surface based classes, make adjustments to WKXReader classes
The repo had been dormant since 2022-05; CI no longer ran end-to-end on
any current GitHub Actions runner image:

  - actions/checkout@v2 + actions/setup-dotnet@v1 are deprecated
  - .NET 5 SDK is EOL and no longer installed on hosted runners
  - .NET Core 3.1 dependencies of the NetTopologySuite submodule cannot
    be loaded on Ubuntu 24.04 (libssl1.1 removed) or arm64 macOS
  - Test project targeted the deprecated 'net5' shorthand TFM
  - GITHUB_ACTION env-var produces the synthetic value '__run' for
    unnamed shell steps, breaking the NTS version-suffix template

Mirror the modernization upstream NetTopologySuite recently adopted:

  - .github/workflows/dotnet.yml: setup-dotnet@v5 with .NET 10, matrix
    over ubuntu/windows/macOS, --blame-hang-timeout 120s so hung tests
    fail loudly instead of silently blocking CI, -p:EnableApiCompat=false
    to skip the legacy Microsoft.DotNet.ApiCompat tool on the submodule
    (its netcoreapp3.1 runtime doesn't load on current runners)
  - test/NetTopologySuite.Curve.Test.csproj: TFM net5 -> net10.0; NUnit
    3.12 -> 3.14, NUnit3TestAdapter 3.16 -> 4.5, Microsoft.NET.Test.Sdk
    16.5 -> 17.10; add System.Runtime.Serialization.Formatters for
    BinaryFormatter on .NET 10
  - test/...CurveGeometryImplTest.cs: wrap BinaryFormatter in
    #pragma warning disable SYSLIB0011 and enable the unsafe-formatter
    AppContext switch (mirrors the upstream develop pattern)
  - src/Directory.Build.props: $(GITHUB_ACTION) -> $(GITHUB_RUN_ID)
    so 'ci.github.<id>' is a valid SemVer build-metadata segment
  - src/.../NetTopologySuite.Curve.csproj: netStandard2.0 -> netstandard2.0

No semantic changes to the library or tests; this is purely the
infrastructure needed to *see* what the current state actually does on
modern tooling. Several existing tests are known to fail or hang under
this configuration — exposing those failures is the point.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Curve vs Curved isn't cosmetic — it's type semantics:

  - Curve   = noun, first-class geometry family (OGC SFA / JTS idiom).
  - Curved  = adjective, implies a modifier layer.

The public surface and the package name are already Curve (no 'd'); the
folder names and one stray .sln/csproj path string are the last residue
of the older naming. Leaving them in baked the wrong abstraction level
into every grep, every project explorer view, every PR diff, in
perpetuity.

What this PR does

  1. Rename via git mv (history-preserving):
       src/NetTopologySuite.Curved/        -> src/NetTopologySuite.Curve/
       test/NetTopologySuite.Curved.Test/  -> test/NetTopologySuite.Curve.Test/
  2. Update the three path strings that referenced the old folder names:
       NetTopologySuite.Curve.sln (two Project lines)
       test/.../NetTopologySuite.Curve.Test.csproj (one ProjectReference)
  3. Add src/NetTopologySuite.Curve.Analyzers/ — a Roslyn analyzer that
     reports NTSC0001 (severity: Error) on any declared identifier whose
     CamelCase parts include the word 'curved' (case-insensitive).
     Matches: Curved, CurvedFoo, FooCurved, _curvedField, curvedLocal.
     Doesn't match: Curve, Curves, CurveData.
  4. Wire the analyzer into both csprojs as a build-time check so
     reintroduction breaks the build instead of slipping through review.

The analyzer fires on declarations only — namespaces, named types,
methods (non-accessor), properties, fields, events, parameters,
locals, and using-alias directives. Property accessor methods
(get_X / set_X) are filtered out so reporting the property itself
isn't duplicated.

After this PR there is no remaining 'Curved' token in the codebase, and
no future PR can introduce one without explicit pragma suppression.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F-CP (locationtech/jts#1195, Phase 1: Foundations) is the gating
sub-issue of the SFA Curve Awareness epic — every Phase-2 TAG that
needs to talk about a curved boundary, area, or validity depends on
CurvePolygon exposing its shell and holes as Curves rather than
collapsing them to flat LinearRings.

On the JTS side this is a real implementation problem because
Polygon.getExteriorRing() is typed LinearRing; the epic's §7 risk #1
(FCP-DOVE) tracks the option-A/B/C dovetail decision for the legacy
accessor.

On the C# side, the dovetail does not exist. NTS.Curve's CurvePolygon
extends Surface<Curve>, the generic abstract base on FObermaier's
enhancement/curved branch (NTS PR #526):

  public class CurvePolygon : Surface<Curve>, ILinearizable<Polygon>
  {
      public override Curve ExteriorRing { get; }
      public override Curve GetInteriorRingN(int index) { ... }
  }

This is, in effect, the endpoint Option-B-on-Polygon would have
reached if the legacy LinearRing-typed accessor had never existed —
so it never has to be widened, and there is no blast-radius count to
do. Four of six F-CP sub-TAGs (FCP-S, FCP-H, FCP-CP, FCP-TL) are
green by architecture; the two reader/writer-dependent ones
(FCP-MEM, FCP-WKT) turn out to be green out of the box too, against
WKTReaderEx / WKTWriterEx on the enhancement/curved submodule HEAD.

What this PR adds

  - docs/SPEC_F_CP.md — the C# companion to the JTS-side
    SPEC_F_CP.md, sub-TAG by sub-TAG mapping plus the architectural
    note explaining why FCP-DOVE is not applicable here.
  - test/NetTopologySuite.Curve.Test/CurveAwareness/
    CurvePolygonStructuralSpec.cs — ten assertions across the six
    sub-TAGs, plus the FCP-DOVE meta-test that pins the
    Surface<Curve> typing via reflection so any future widening of
    ExteriorRing back to LinearRing forces the dovetail decision
    instead of slipping through review.

Empirical status as of 2026-05-15: all ten tests green against
enhancement/curved HEAD (2772c9b3) + the CI wake-up and rename PRs
(#7 and #8). F-CP needs no implementation work; this PR is the
regression net that pins it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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