Skip to content

mcstas-pygen: do not crash on instrument parameters without default value - #2762

Merged
willend merged 1 commit into
mccode-dev:mainfrom
tkittel:pygen-param-no-default
Oct 6, 2026
Merged

willend merged 1 commit into
mccode-dev:mainfrom
tkittel:pygen-param-no-default

Conversation

@tkittel

@tkittel tkittel commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Another pygen bug, discovered while working on the new ESS instrument repo tool.

Claude:

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:

mcstas-pygen crashes with a segmentation fault on any instrument with a parameter without default value, e.g.

DEFINE INSTRUMENT T(x, string s, int n, double d=1)

In cogen_decls (mccode/src/pygen.c.in), the default value of each instrument parameter was passed to exp_tostring (or pygen_pyexp for string parameters) without checking that there is one. For a parameter without default value, default_value is NULL. The C code generator checks isoptional for this (in cogen.c.in).

With this PR, such parameters are written without value=, which in McStasScript means "no default value", so the translation is faithful:

x = instr.add_parameter('double', 'x', comment='Parameter type (double) added by McCode py-generator')
s = instr.add_parameter('string', 's', comment='Parameter type (string) added by McCode py-generator')
n = instr.add_parameter('int', 'n', comment='Parameter type (int) added by McCode py-generator')
d = instr.add_parameter('double', 'd', value=1, comment='Parameter type (double) added by McCode py-generator')

Parameters with a default value are written as before. A string parameter without default value is now also distinguishable from one with the default value "" (value='""').

This came up in a tool for the ESS instrument repositories (mcstas-ess-instr-citool), which translates .instr files with mcstas-pygen to check rules on the instruments, such as that all instrument parameters have default values; that check was impossible, since mcstas-pygen crashed on exactly the instruments breaking the rule.

Test results, with mcstas-pygen built from main and from this PR (on Linux, with the component library of McStas 3.9.1 from conda-forge):

  • The example above (with parameters of all types, with and without default values): main crashes (exit code 139); this PR writes the code shown above. With McStasScript 0.0.94, make() of the generated script gives the parameters without default value the value None, and write_full_instrument() writes them back without default value (double x,).
  • All 332 instruments in mcstas-comps/examples: 331 give identical output with main and this PR (ignoring date and path lines). One (Test_Dispersion_relation_TAS) fails with both, for an unrelated reason (its SEARCH SHELL statement runs python, which was not on the path in my test environment; mcstas fails the same way).

Declaration of use of AI-tools

  • Please add a checkmark here if you used AI-tools during the work for this contribution

  • Furter, please describe how / where and for what the tools were used:

    Claude Code (Anthropic, model Claude Opus 5.5) was used, under my direction, to find the cause of the crash, to write the patch, and to run the tests described above.


Development OS / boundary conditions

Please describe what OS you developed and tested your additions on, and if any special dependencies are required:

Developed and tested on Linux (Ubuntu, x86_64), building mcstas-pygen with CMake (-DBUILD_MCSTAS=ON), with bison, flex and the C compiler from conda-forge. Not tested on Windows or macOS; the change only affects the generated Python code for instrument parameters. No new dependencies.


PR Checklist for contributing to McStas/McXtrace

For a coherent and useful contribution to McStas/McXtrace, please fill in relevant parts of the checklist:

  • My contribution includes patches to an existing component file

    • Not relevant.
  • My contribution includes patches to an existing instrument file

    • Not relevant.
  • My contribution includes a new component file

    • Not relevant.
  • My contribution includes a new instrument file

    • Not relevant.
  • My work touches the code-generator in mccode/src

    • I have added reasoning and documentation for the change through an ADR record in our GRAMMAR section

      Not done: this is a bug fix in the Python code generator (pygen.c.in), without changes to the grammar. Happy to add one if wanted.

    • I am attaching test output in the comments

      The test results are described above (the crash before and the output after, and the comparison for all instruments in mcstas-comps/examples).

  • My work touches / adds to the runtime lib code (.c,.h etc in multiple locations

    • Not relevant.
  • My PR is meant to fix a specific, existing issue

    • Not relevant (no issue exists for this).
  • My contribution contains something else

    • Not relevant.

🤖 Generated with Claude Code

…alue

For an instrument parameter without default value, cogen_decls in pygen.c.in
passed the NULL default_value to exp_tostring (or pygen_pyexp for string
parameters), and mcstas-pygen crashed with a segmentation fault. Such
parameters are now written without value= (instr.add_parameter('double',
'x', comment=...)), i.e. without default value in McStasScript too, like
the C code generator does (which checks isoptional).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@willend

willend commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@tkittel thanks, this should make it to 3.9.2 that I am about to put together. (Pending succesful runs of the testsuites, currently ongoing as https://github.com/mccode-dev/McCode/actions/runs/37443619795 and https://github.com/mccode-dev/McCode/actions/runs/37443654914)

@willend
willend merged commit 89e2602 into mccode-dev:main Oct 6, 2026
15 checks passed
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