diff --git a/docs/how-to/write_docs.rst b/docs/how-to/write_docs.rst index 3d0f9f0fa..37fc545e9 100644 --- a/docs/how-to/write_docs.rst +++ b/docs/how-to/write_docs.rst @@ -68,8 +68,8 @@ For further documentation on needextends please `look here NeedextendLocation: + """Return the source location used for every diagnostic of one directive.""" + return needextend["docname"], needextend["lineno"] + + +def _warn(message: str, location: NeedextendLocation) -> None: + """Report a needextend violation at the directive, not at its target need.""" + log_warning(logger, message, "needextend", location=location) + + +def _verify_needs_are_in_document( + needs: list[NeedItem], location: NeedextendLocation +) -> None: + """Report selected needs outside the directive's source document. + + Both ID and expression targets resolve to need records. Validating that shared + representation keeps the document-boundary policy identical for both syntaxes. + """ + remote_ids = {need["id"] for need in needs if need["docname"] != location[0]} + if remote_ids: + _warn( + "Needextends may only modify needs in the current document. " + f"Matching needs: {', '.join(sorted(remote_ids))}.", + location, + ) + -def score_extend_needs_data_func( # noqa: C901 +def _fetch_needs( + all_needs: NeedsMutable, + needextend: NeedsExtendType, + needs_config: NeedsSphinxConfig, +) -> list[NeedItem]: + """Resolve either supported target syntax to the needs it selects.""" + location = _location(needextend) + + if needextend["filter_is_id"]: + # ``.. needextend:: NEED_ID`` has no expression to constrain, so inspect + # its resolved target in the shared document-boundary check below. + need_id = needextend["filter"] + try: + return [all_needs[need_id]] + except KeyError: + _warn( + f"Provided id {need_id!r} for needextend does not exist.", + location, + ) + return [] + + else: + need_filter = needextend["filter"] + + if "c.this_doc()" not in need_filter: + _warn( + "needextend in S-CORE must always be used per document only. " + "Please add 'c.this_doc()' to the needextend to limit its effects to the correct document. " + "See https://eclipse-score.github.io/docs-as-code/main/how-to/write_docs.html#needextend for more information.", + location, + ) + + try: + return filter_needs_mutable( + all_needs, + needs_config, + need_filter, + location=location, + origin_docname=location[0], + ) + except Exception as e: + _warn(f"Invalid filter {need_filter!r}: {e}", location) + return [] + + +def _validate_list_modifications( + need: NeedItem, + needextend: NeedsExtendType, + location: NeedextendLocation, +) -> None: + """Reject destructive changes to link lists, which would erase traceability.""" + for _, action, value in needextend["list_modifications"]: + replaces_or_deletes_links = action in { + ExtendType.REPLACE, + ExtendType.DELETE, + } and isinstance(value, LinksLiteralValue | LinksFunctionArray) + if replaces_or_deletes_links: + _warn( + f"Error when extending need: {need['id']}. " + "Replace or Delete action is not allowed via needextends.", + location, + ) + + +def _validate_field_modifications( + need: NeedItem, + needextend: NeedsExtendType, + location: NeedextendLocation, +) -> None: + """Reject field changes that discard data or append to scalar fields.""" + for option_name, action, value in needextend["modifications"]: + is_scalar_append = ( + action == ExtendType.APPEND + and isinstance(value, FieldLiteralValue) + and isinstance(value.value, str) + ) + is_supported_replacement = action == ExtendType.REPLACE and ( + value is None or isinstance(value, FieldLiteralValue | FieldFunctionArray) + ) + + if action == ExtendType.DELETE: + _warn( + f"Error when extending need: {need['id']}. " + "Delete action is not allowed via needextends.", + location, + ) + elif is_scalar_append: + _warn( + f"Error when extending need: {need['id']}. " + "Append action is not allowed via needextends on 'string type options'.", + location, + ) + elif is_supported_replacement and need[option_name]: + _warn( + f"Error when extending need: {need['id']}. " + "Replacing of options that are already set is not allowed via needextends.", + location, + ) + + +def _ensure_non_destructive_changes( + need: NeedItem, + needextend: NeedsExtendType, + location: NeedextendLocation, +) -> None: + """Apply SCORE's non-destructive extension policy to one selected need.""" + if need["is_external"]: + _warn( + f"Error when extending need: {need['id']}. " + "It is not allowed to modify external needs via needextend", + location, + ) + _validate_list_modifications(need, needextend, location) + _validate_field_modifications(need, needextend, location) + + +def score_extend_needs_data_func( all_needs: NeedsMutable, extends: dict[str, NeedsExtendType], needs_config: NeedsSphinxConfig, ): - """Use data gathered from needextend directives to modify fields of existing needs.""" - # regardless of parallel build worker completion order. - sorted_extends = sorted(extends.values(), key=lambda x: (x["docname"], x["lineno"])) - - current_needextend: NeedsExtendType - for current_needextend in sorted_extends: - need_filter = current_needextend["filter"] - location = (current_needextend["docname"], current_needextend["lineno"]) - - # ╓ ╖ - # ║ This is currently as a grace period still allowed, but ║ - # ║ will be forbiden in future releases ║ - # ╙ ╜ - # if "c.this_doc()" not in need_filter: - # error_msg = "Potentially altering needs outside of the document is not allowed. Please add 'c.this_doc()' to the needextend to limit it to only needs in the same document" - # log_warning(logger, error_msg, "needextend", location=location) - - if current_needextend["filter_is_id"]: - try: - found_needs = [all_needs[need_filter]] - except KeyError: - error = f"Provided id {need_filter!r} for needextend does not exist." - if current_needextend["strict"]: - raise NeedsInvalidFilter(error) from KeyError - log_warning(logger, error, "needextend", location=location) - continue - else: - try: - found_needs = filter_needs_mutable( - all_needs, - needs_config, - need_filter, - location=location, - origin_docname=current_needextend["docname"], - ) - except Exception as e: - log_warning( - logger, - f"Invalid filter {need_filter!r}: {e}", - "needextend", - location=location, - ) - continue - for found_need in found_needs: - if found_need["is_external"]: - log_warning( - logger, - f"Error when extending need: {found_need['id']}. " - + "It is not allowed to modify external needs via needextend", - "needextend", - location, - ) - # Work in the stored needs, not on the search result - need = all_needs[found_need["id"]] - - location = ( - current_needextend["docname"], - current_needextend["lineno"], - ) + """Validate SCORE's needextend policy, then let Sphinx-Needs apply it. + + This wrapper intentionally only reports violations. The unmodified directives + are still passed to Sphinx-Needs so its normal processing and diagnostics remain + authoritative. + """ + # Sphinx-Needs applies extensions in source order as well. Matching that order + # keeps warning output stable and mirrors the later application order. + ordered_extends = sorted(extends.values(), key=_location) + + for needextend in ordered_extends: + needs = _fetch_needs(all_needs, needextend, needs_config) + _verify_needs_are_in_document(needs, _location(needextend)) + + for n in needs: + _ensure_non_destructive_changes(n, needextend, _location(needextend)) - for _, etype, link_value in current_needextend["list_modifications"]: - match (etype, link_value): - case ( - ExtendType.REPLACE | ExtendType.DELETE, - LinksLiteralValue() | LinksFunctionArray(), - ): - # Replacing / Deleting links is not allowed - error_msg = ( - f"Error when extending need: {need['id']}. " - "Replace or Delete action is not allowed via needextends." - ) - # logger.warning_for_need(current_needextend["id"], error_msg) - log_warning(logger, error_msg, "needextend", location=location) - - for option_name, etype, field_value in current_needextend["modifications"]: - if etype == ExtendType.DELETE: - error_msg = ( - f"Error when extending need: {need['id']}. " - "Delete action is not allowed via needextends." - ) - log_warning(logger, error_msg, "needextend", location=location) - match (etype, field_value): - case (ExtendType.APPEND, FieldLiteralValue()): - if isinstance(field_value.value, str): - error_msg = ( - f"Error when extending need: {need['id']}. " - "Append action is not allowed via needextends on 'string type options'." - ) - log_warning( - logger, error_msg, "needextend", location=location - ) - - case ( - ExtendType.REPLACE, - None | FieldLiteralValue() | FieldFunctionArray(), - ): - if need[option_name]: - error_msg = f"Error when extending need: {need['id']}. Replacing of options that are already set is not allowed via needextends." - - log_warning( - logger, error_msg, "needextend", location=location - ) return original_function(all_needs, extends, needs_config) diff --git a/src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst b/src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst index 2bc22385b..007ded5fe 100644 --- a/src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst +++ b/src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst @@ -73,8 +73,8 @@ :expect: Error when extending need: stkh_req__test__need_extends_3. Append action is not allowed via needextends on 'string type options' -.. This will be activated once we have activated the c.this_doc() check aswell -.. #EXPECT[+2]: Potentially altering needs outside of the document is not allowed. Please add 'c.this_doc()' to the needextend to limit it to only needs in the same document +.. A needextend must explicitly be limited to needs in its own document. -.. .. needextend: id == 'stkh_req__test__need_extends_1' -.. :security: QM +.. needextend:: id == 'stkh_req__test__need_extends_1' + :security: QM + :expect: needextend in S-CORE must always be used per document only. Please add 'c.this_doc()' to the needextend to limit its effects to the correct document. diff --git a/src/extensions/score_metamodel/tests/test_check_needs_extends.py b/src/extensions/score_metamodel/tests/test_check_needs_extends.py new file mode 100644 index 000000000..102d35324 --- /dev/null +++ b/src/extensions/score_metamodel/tests/test_check_needs_extends.py @@ -0,0 +1,115 @@ +# ******************************************************************************* +# Copyright (c) 2026 Contributors to the Eclipse Foundation +# +# See the NOTICE file(s) distributed with this work for additional +# information regarding copyright ownership. +# +# This program and the accompanying materials are made available under the +# terms of the Apache License Version 2.0 which is available at +# https://www.apache.org/licenses/LICENSE-2.0 +# +# SPDX-License-Identifier: Apache-2.0 +# ******************************************************************************* + +from typing import cast +from unittest.mock import Mock + +from score_metamodel.checks import check_needs_extends +from sphinx_needs.data import NeedsExtendType, NeedsMutable +from sphinx_needs.need_item import NeedItem + +# TODO(#688): Replace these mocked tests with multi-file RST tests once the +# RST runner can build complete cross-document test cases. +CROSS_DOCUMENT_WARNING = ( + "Needextends may only modify needs in the current document. " + "Matching needs: remote-need." +) + + +def _need(need_id: str, document: str) -> NeedItem: + """Create the minimal need record used by the location check.""" + return cast( + NeedItem, + {"id": need_id, "docname": document, "is_external": False}, + ) + + +def _needextend(target: str, *, target_is_id: bool) -> NeedsExtendType: + """Create a needextend with no modifications; only target resolution matters.""" + return cast( + NeedsExtendType, + { + "docname": "local", + "lineno": 1, + "filter": target, + "filter_is_id": target_is_id, + "strict": False, + "list_modifications": [], + "modifications": [], + }, + ) + + +def _run_check( + monkeypatch, + all_needs: NeedsMutable, + needextend: NeedsExtendType, + *, + filter_matches: list[NeedItem] | None = None, +) -> tuple[Mock, Mock]: + """Run the wrapper with isolated Sphinx-Needs dependencies.""" + warnings = Mock() + original_function = Mock() + monkeypatch.setattr(check_needs_extends, "log_warning", warnings) + monkeypatch.setattr(check_needs_extends, "original_function", original_function) + if filter_matches is not None: + monkeypatch.setattr( + check_needs_extends, + "filter_needs_mutable", + Mock(return_value=filter_matches), + ) + + check_needs_extends.score_extend_needs_data_func( + all_needs, {"needextend-1": needextend}, Mock() + ) + return original_function, warnings + + +def _assert_cross_document_warning(warnings: Mock) -> None: + messages = [call.args[1] for call in warnings.call_args_list] + assert CROSS_DOCUMENT_WARNING in messages + + +def test_filter_reports_cross_document_or_match_without_changing_directive( + monkeypatch, +): + local_need = _need("local-need", "local") + remote_need = _need("remote-need", "remote") + all_needs = cast( + NeedsMutable, + {"local-need": local_need, "remote-need": remote_need}, + ) + needextend = _needextend("c.this_doc() or id == 'remote-need'", target_is_id=False) + + original_function, warnings = _run_check( + monkeypatch, + all_needs, + needextend, + filter_matches=[local_need, remote_need], + ) + + assert original_function.call_args.args[1]["needextend-1"] is needextend + _assert_cross_document_warning(warnings) + + +def test_id_shorthand_reports_cross_document_match(monkeypatch): + remote_need = _need("remote-need", "remote") + all_needs = cast(NeedsMutable, {"remote-need": remote_need}) + + _, warnings = _run_check( + monkeypatch, + all_needs, + _needextend("remote-need", target_is_id=True), + ) + + _assert_cross_document_warning(warnings) diff --git a/src/extensions/score_metamodel/tests/test_rules_file_based.py b/src/extensions/score_metamodel/tests/test_rules_file_based.py index a9ef1e778..4d47fe90d 100644 --- a/src/extensions/score_metamodel/tests/test_rules_file_based.py +++ b/src/extensions/score_metamodel/tests/test_rules_file_based.py @@ -23,6 +23,7 @@ from sphinx.testing.util import SphinxTestApp from sphinx_needs.data import NeedsExtendType, SphinxNeedsData from sphinx_needs.need_item import NeedItem +from sphinx_needs.needs_schema import FieldLiteralValue from sphinx_needs.views import NeedsView from score_pytest.attribute_plugin import apply_test_metadata @@ -223,6 +224,17 @@ def _collect_warnings(app: SphinxTestApp) -> list[str]: return warnings +def _get_expectations(need: NeedItem | NeedsExtendType, option_name: str) -> list[str]: + """Get test warning annotations from a need or needextend directive.""" + if isinstance(need, NeedItem): + return cast("list[str]", need.get(option_name) or []) + + for name, _, value in need["modifications"]: + if name == option_name and isinstance(value, FieldLiteralValue): + return cast("list[str]", value.value or []) + return [] + + def _check_need_warnings( rst_data: RstData, need: NeedItem | NeedsExtendType, warnings: list[str] ) -> None: @@ -234,7 +246,7 @@ def _check_need_warnings( line_nr = need.get("lineno") - for raw in cast("list[str]", need.get("expect") or []): + for raw in _get_expectations(need, "expect"): expected = raw.strip() if warning_matches(rst_data, line_nr, expected, warnings): continue @@ -248,7 +260,7 @@ def _check_need_warnings( pytrace=False, ) - for raw in cast("list[str]", need.get("expect_not") or []): + for raw in _get_expectations(need, "expect_not"): not_expected = raw.strip() unexpected = warning_matches(rst_data, line_nr, not_expected, warnings) if not unexpected: