Keep function body tail remarks in place

[HassanAkbar]

Tail comments inside FUNCTION, PROCEDURE, and RULE bodies get lost or misplaced during formatting. This fixes their attachment and their emission.

The bug

For a -- comment written on its own line inside a body:

• Comments before a statement were dropped from the output.
• Comments inside IF branches vanished entirely.
• Comments in REPEAT bodies moved to just before END_REPEAT.
• The last comment line could leak past END_FUNCTION as a trailer.

On one large production STEP schema, only 19 of 64 body comments survived formatting.

Example, before this fix:

FUNCTION check_steps(input : INTEGER) : LOGICAL;
  ...
  -- STEP-1
  -- accumulate the input value
  total := total + input;

formatted with the STEP-1 block missing. It now formats with the comments exactly where they were written.

The fix

• A body comment now attaches to the statement that follows it.
• Attachment is region-aware. THEN and ELSE branches don't cross.
• A new optional RemarkInfo#placement field marks these as "leading".
• The formatter emits leading remarks right above their statement.
• nil placement keeps exact legacy behavior. Old caches load unchanged.
• REPEAT/ALIAS block ends now emit only tagged and terminal remarks, indented with the body.
• A terminal body comment attaches to its enclosing node deterministically instead of via the nearest-line heuristic.

Evidence

• Full suite: 1482 examples, 0 failures.
• The production schema above: all 64 body comments now render, none did fully before.
• Cache round-trip output is byte-identical to a direct parse.
• no_remarks output is unchanged.
• The new spec (formatter_function_body_remarks_spec.rb) covers all four failure patterns plus ELSE, terminal, and schema-level edge cases.

Notes for reviewers

• The mathematical_functions_schema YAML snapshot changed. Zero remark texts were lost; 108 were recovered. Those 108 previously fell off statement nodes during serialization because statements never mapped untagged_remarks.
• Nine statement classes now serialize untagged_remarks; a new Model::Statement marker identifies executable statements.
• If now declares else_statements as a traversed collection.
• Known limits, unchanged from before: a comment sitting directly before ELSE or END_* with no following statement keeps the old behavior, and CASE bodies are untouched.

[Copilot]

Pull request overview

This PR fixes a long-standing formatter/serializer issue where own-line -- tail remarks inside FUNCTION/PROCEDURE/RULE bodies were being dropped, moved across IF/ELSE boundaries, or emitted in the wrong scope. It introduces a deterministic attachment strategy for “body comments” by attaching them to the following executable statement within the correct statement region and emitting them as leading remarks.

Changes:

• Add RemarkInfo#placement (with "leading") and a Model::Statement marker to distinguish executable statements and drive region-aware attachment/emission.
• Update remark attachment/indexing so own-line body comments attach to the next statement in the same region (including ELSE awareness), with deterministic handling for terminal comments.
• Emit leading statement remarks above their owning statement and adjust ALIAS/REPEAT block-end remark emission to avoid losing or misplacing trailing remarks.

Reviewed changes

Copilot reviewed 21 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
spec/syntax/remark_formatted.exp	Updates expected formatted EXPRESS output for remark indentation/placement in ALIAS/REPEAT bodies.
spec/syntax/mathematical_functions_schema/mathematical_functions_schema.yaml	Snapshot update reflecting recovered/moved untagged_remarks and new placement: leading serialization.
spec/fixtures/function_body_remarks.exp	Adds a fixture schema exercising leading body comments in IF/ELSE and REPEAT contexts.
spec/expressir/express/formatter_function_body_remarks_spec.rb	Adds coverage for leading body comment retention and key edge cases (ELSE, inline ELSE, repeat terminal comment).
lib/expressir/model/statements/*.rb	Marks executable statements (include Statement) and maps untagged_remarks for additional statement classes.
lib/expressir/model/remark_info.rb	Adds placement attribute and leading? helper; includes YAML/XML serialization for placement.
lib/expressir/model/concerns.rb	Introduces the Statement marker concern.
lib/expressir/model.rb	Autoloads the new Statement concern.
lib/expressir/express/remark_attacher.rb	Implements region-aware body-comment targeting (THEN vs ELSE), adds ELSE boundary detection, and records placement on attached remarks.
lib/expressir/express/node_position_index.rb	Tracks owner + collection for nodes to support region-aware remark attachment.
lib/expressir/express/formatters/statements_formatter.rb	Switches ALIAS/REPEAT to emit only appropriate block-end remarks with correct indentation.
lib/expressir/express/formatters/remark_formatter.rb	Adds leading-statement remark emission and block-end remark filtering logic.
lib/expressir/express/formatter.rb	Prepends leading remarks when formatting statement nodes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lib/expressir/express/remark_attacher.rb:34

• Set from Ruby stdlib is used in this class (e.g., @attached_spans = Set.new, ancestors = Set.new) but the file doesn't require it. This makes RemarkAttacher fail to load in contexts where set hasn't already been required (which also contradicts the nearby note about modules being loadable independently).

      # Collections holding executable statements — the regions a leading
      # body comment can belong to.
      STATEMENT_REGIONS = %i[statements else_statements].freeze

      # Matches the ELSE keyword opening a line or following a statement
      # terminator, optionally trailed by an inline tail remark.
      ELSE_BOUNDARY = /\A(?:.*;)?\s*ELSE(?:\s*--.*)?\z/i

[Copilot]

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lib/expressir/model/model_element.rb:308

• ModelElement#to_s now forces no_remarks: true, which changes the implicit output of string interpolation ("#{element}") to always drop remarks. If to_s is part of the public surface (used by callers to render full EXPRESS including remarks), this is a breaking behavioral change that’s not obviously related to this PR’s remark-placement fix.

Consider keeping to_s as the full formatter (default no_remarks: false) and letting callers explicitly request remark-free output via format(no_remarks: true).

      # No-arg delegator so string interpolation ("#{element}") still
      # produces EXPRESS source rather than the default Object#to_s.
      # set default formatter to no_remarks: true
      def to_s
        format(no_remarks: true)
      end

[ronaldtse]

@HassanAkbar is this ready to be merged? We need it released. Thanks!

[HassanAkbar]

@ronaldtse While testing I found a few edge cases, they are fixed in this PR now:

• Comment right before ELSE was dropped
• Comment before END_IF/END_FUNCTION moved after the keyword
• Comments in CASE actions and OTHERWISE were dropped or misplaced
• Comment before END_CASE, END; and a rule's WHERE was dropped
• Inline comments after a statement (x := 1; -- note) were dropped
• to_s was returning output without remarks

Once the Copilot review is green, this PR is ready to merge on my end.

These are not implemented here, I'll create separate PRs for them:

• Comments between two declarations at schema level are still dropped. The schema formatter emits declarations in type-group order, not source order, so attaching such a comment to the next declaration would move it elsewhere in the output. That needs the ordering handled first.
• Embedded (* ... *) comments are not placed yet, only -- comments are.
• Two complete declarations written on one physical line can misplace a body-tail comment. It needs proper tokenising instead of line matching, same as the embedded comments above.

All three behave the same as main today, so nothing regressed here.

[Copilot]

Pull request overview

Copilot reviewed 33 out of 35 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (2)

spec/expressir/express/remark_attacher_performance_spec.rb:68

• This example uses wall-clock timing (Benchmark.realtime) to detect quadratic behavior, which can be flaky across CI runners and load. The previous example already asserts the key invariant (build_active_scope_map is built once); consider replacing this timing assertion with another deterministic call-count assertion on the larger input, and drop the Benchmark dependency.

      small_time = Benchmark.realtime { Expressir::Express::Parser.from_exp(source) }
      big_time = Benchmark.realtime { Expressir::Express::Parser.from_exp(big) }

      expect(big_time).to be < (small_time * 12)

spec/expressir/express/remark_attacher_performance_spec.rb:2

• After removing the Benchmark-based timing assertion, this require becomes unused. It can be dropped to keep the spec lean and avoid loading extra stdlib code.

This issue also appears on line 65 of the same file.

require "benchmark"