Skip to content

[tools] Preserve negated miaou list wrapper#1768

Open
relokin wants to merge 7 commits intoherd:masterfrom
relokin:miaou-fix
Open

[tools] Preserve negated miaou list wrapper#1768
relokin wants to merge 7 commits intoherd:masterfrom
relokin:miaou-fix

Conversation

@relokin
Copy link
Copy Markdown
Member

@relokin relokin commented Mar 30, 2026

Keep negated composite lists wrapped in a dedicated node instead of reusing the original list operator.

Without this, miaou can flatten the negated sub-list into the enclosing conjunction and lose the intended 'it is not the case that all of the following apply' structure.

For example:

let rel1 = [A]; rel2; [B \ range([C]; rel; [D])]

should render as:

  - E1 is an A effect.
  - E1 is rel2-before E2.
  - E2 is a B effect.
  - It is not the case that all of the following apply:
    - E2 is a D effect.
    - E3 is rel-before E2.
    - E3 is a C effect.

Assisted-by: OpenAI Codex

@relokin
Copy link
Copy Markdown
Member Author

relokin commented Apr 7, 2026

@maranget @HadrienRenaud, this PR adds regressions for miaou. On my laptop the overhead is a few seconds but I wanted to check that you are ok with that.

@HadrienRenaud
Copy link
Copy Markdown
Collaborator

Hi @relokin,
Thanks for checking. It's fine by me.

Copy link
Copy Markdown
Collaborator

@fsestini fsestini left a comment

Choose a reason for hiding this comment

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

Thanks @relokin . I see why this fixes the issue, but the representation of negated lists still feels slightly off to me.

As far as I can tell, the original issue is that flatten_out merges nested List nodes when their operators match. This patch avoids that by rewriting a negated composite list to List (Diff, ...).

That works in practice, but it means the op field is no longer describing the meaning of the node. In the case of notItem, the list being produced is not necessarily a difference, but the op field will be set to Diff regardless; that is because Diff is just being used as a sentinel to opt out of flattening. I think that makes the code harder to understand locally, because the reason for setting Diff here only becomes clear once the reader knows about this specific bug and how flatten_out happens to behave.

Since the intent here is to preserve negated composite lists as their own node, IMO that would be better represented explicitly in t, rather than encoded indirectly through the AST.op2 field.

For example, this could be a dedicated constructor for negated t terms:

type t =
  ...
  | Negated of t
  ...

or a separate flattenable flag on the existing List constructor that controls whether that list participates in flattening:

type t =
  ...
  | List of { op : AST.op2; flattenable : bool; intro_txt : string; sep_txt : string * string; children : t list; }
  ...

then functions like flatten_out and rm_dump can selectively ignore negated/non-flattenable lists:

let rec flatten_out = function
  | List ({ op = Inter|Union|Seq ; flattenable = true; intro_txt ; sep_txt ; children } as l) ->
     List { l with children = flatten_op l.op l.children }
  | List l ->
     List { l with children = List.map flatten_out l.children }
  | ...

relokin added 5 commits April 13, 2026 16:31
Assisted-by: OpenAI Codex
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Assisted-by: Codex AI
Add a `flattenable` flag to miaou's internal `List` nodes so
flattening can be controlled per list.

This lets miaou preserve wrapper lists that carry important prose
structure, instead of flattening them unconditionally with surrounding
conjunctions or unions. In particular, synthetic lists introduced by
`DiffPair` and `IfCond` formatting, and negated composite lists, can be
kept intact when their intro text must remain visible.

The flattening pass now only merges lists whose `flattenable` flag is
set, while existing ordinary relation and event lists remain
flattenable by default.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Assisted-by: Codex AI
Keep negated composite lists wrapped in a dedicated node instead of
reusing the original list operator.

Without this, miaou can flatten the negated sub-list into the enclosing
conjunction and lose the intended 'it is not the case that all of the
following apply' structure.

For example:

  let rel1 = [A]; rel2; [B \ range([C]; rel; [D])]

should render as:
  - E1 is an A effect.
  - E1 is rel2-before E2.
  - E2 is a B effect.
  - It is not the case that all of the following apply:
    - E2 is a D effect.
    - E3 is rel-before E2.
    - E3 is a C effect.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Assisted-by: OpenAI Codex
Drop the special-case rewrite for differences of the form
`r \ (c ; r)`.

That rewrite tried to turn the difference into a conjunction using
the negation of `c`, but it can lose the intermediate witness event
and change the meaning of the prose. For example, `po \ (po; po)`
can collapse to a contradictory `po` / `not po` description instead
of expressing that there is no intervening `po` step.

After this change, `tr_diff` only applies the generic transformation
`A \ B -> A and not B` when the whole right-hand side `B` has a
usable negated prose form. Otherwise it keeps the difference as a
`DiffPair`, which preserves the full structure of the excluded path.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Assisted-by: Codex AI
@fsestini fsestini self-requested a review April 24, 2026 14:50
Copy link
Copy Markdown
Collaborator

@fsestini fsestini left a comment

Choose a reason for hiding this comment

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

Thanks @relokin for addressing my concerns. I left a couple of minor non-blocking thoughts/comments, but this LGTM already.

Comment thread tools/miaou.ml Outdated
Comment thread tools/miaou.ml Outdated
@fsestini
Copy link
Copy Markdown
Collaborator

Thanks @relokin , feel free to squash/rebase/merge at any time!

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.

3 participants