Readonly builtin #320
Readonly builtin #320
Conversation
It is found that PrintVariablesContext can be used to print not only variables but also functions. This commit renames it to PrintContext to reflect the fact.
A reference to an object with a static lifetime should be const. If it is declared static, not only the referenced object but also the reference itself will occupy a memory for a static lifetime.
This commit introduces a new field `builtin_is_significant` to the `PrintContext` struct. When this field is true, the command that invokes the built-in is always printed regardless of the attributes of the variables or functions. This field is true for the export and readonly built-ins, but false for the typeset built-in.
|
The existing content is still valid, so I will repeat it verbatim: WalkthroughThe updates across various shell built-in commands ( Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- yash-builtin/src/export.rs (5 hunks)
- yash-builtin/src/lib.rs (1 hunks)
- yash-builtin/src/readonly.rs (2 hunks)
- yash-builtin/src/typeset.rs (10 hunks)
- yash-builtin/src/typeset/print_functions.rs (12 hunks)
- yash-builtin/src/typeset/print_variables.rs (27 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/readonly-p.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- yash/tests/scripted_test.rs
Additional comments: 53
yash-builtin/src/lib.rs (1)
- 144-148:
Ensure that thereadonly::mainfunction is now properly returning aFutureand that this change is compatible with the rest of the codebase wherereadonly::mainis expected to be called.yash/tests/scripted_test/readonly-p.sh (3)
5-11:
The test case for making a single variable read-only is well-defined and checks the expected behavior correctly.13-20:
The test case for making multiple variables read-only is well-defined and checks the expected behavior correctly.26-32:
The test case for ensuring a read-only variable cannot be reassigned is well-defined and includes an explanation for the expected behavior when thereadonlybuilt-in fails.yash-builtin/src/readonly.rs (3)
17-191: The documentation added for the
readonlybuilt-in is comprehensive and well-structured, providing clear information on its behavior, options, operands, and output. This will greatly enhance the maintainability and usability of the code.222-248: The
mainfunction has been refactored to be asynchronous and now properly handles the execution of thereadonlybuilt-in command. The use ofparseandinterpretfunctions from thetypesetmodule, along with the newPRINT_CONTEXTconstant, shows a good separation of concerns and code reuse.262-268: The test case
builtin_defines_read_only_variablechecks if thereadonlybuilt-in correctly defines a read-only variable. However, the test case seems to be missing an assertion to check if the variable is indeed set as read-only. This should be verified and potentially corrected to ensure the test case is comprehensive.yash-builtin/src/export.rs (5)
26-33: The updated description of the
-poption in theexportbuilt-in command is clear and informative, explaining the behavior with and without operands.43-61: The clarification on operands and standard output behavior when exporting or printing variables is well-documented and improves the understanding of the command's functionality.
80-87: The implementation notes provide valuable insight into the relationship between the
exportandtypesetbuilt-ins, which is useful for developers working with the codebase.95-113: The refactoring to rename
PRINT_VARIABLES_CONTEXTtoPRINT_CONTEXTand the associated updates to structures and functions align with the changes mentioned in the summary and improve code consistency.129-134: The use of
unreachable!macro forCommand::SetFunctionsandCommand::PrintFunctionsvariants ensures that theexportbuilt-in does not handle function-related commands, which is consistent with its intended behavior.yash-builtin/src/typeset.rs (5)
82-86: The documentation mentions that variables are printed in alphabetical order, but there is no code in the provided hunks that shows the implementation of this sorting. Verify that the sorting logic is implemented elsewhere in the codebase.
126-131: The documentation explains the special handling of array variables when printing, including the omission of the typeset command if no options are applied. Ensure that the implementation of this behavior is consistent with the documentation.
177-181: The documentation states that shell functions are printed in alphabetical order. Similar to the sorting of variables, verify that the sorting logic for functions is implemented elsewhere in the codebase.
263-270: The documentation has been updated to reflect the parameterization of common functions to support different behaviors of the built-ins. Ensure that the refactoring has been done correctly and that the parameterization does not introduce any regressions or unexpected behaviors.
482-493: The
executemethod is responsible for executing the command and returning the result or errors. Ensure that the error handling and result output are consistent with the new asynchronous error reporting and result output functions mentioned in the pull request summary.yash-builtin/src/typeset/print_functions.rs (12)
22-30: The
executemethod in thePrintFunctionsstruct has been updated to include a newcontextparameter of type&PrintContext. This change aligns with the summary indicating that theprint_onefunction now uses thecontextparameter to determine options for printing and the significance of the built-in name. The method signature change is consistent with the described enhancements.34-45: The sorting of functions is done using an unstable sort, which is fine for sorting by function names as they are expected to be unique. However, the comment on line 34 indicates a TODO for honoring the collation order in the locale, which is not addressed in this change. This is consistent with the learning that locale-specific sorting is recognized as a potential enhancement but not implemented due to extensive changes required across the codebase.
67-98: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [56-96]
The
print_onefunction has been updated to include thecontextparameter and uses it to determine which options to print and whether the built-in name is significant. This change is in line with the summary of changes and the new tests added to cover different scenarios using thePrintContextstruct. The implementation appears to be correct and matches the intended functionality.
123-127: The test
printing_one_functionchecks the output of theexecutemethod for a single function. The test is straightforward and correctly asserts the expected output. The use ofPRINT_CONTEXTis consistent with the changes made to thePrintFunctionsstruct.147-152: The test
printing_multiple_functionschecks the output of theexecutemethod for multiple functions. The test is well-structured and correctly asserts the expected output. The use ofPRINT_CONTEXTis consistent with the changes made to thePrintFunctionsstruct.173-178: The test
printing_all_functionschecks the output of theexecutemethod when all functions are to be printed. The test correctly asserts that the output is sorted by function name, which is consistent with the implementation in theexecutemethod.192-197: The test
quoting_function_namechecks the output of theexecutemethod for a function with a name that requires quoting. The test correctly asserts the expected output, including the use of thefunctionkeyword and quoting of the function name.211-215: The test
printing_readonly_functionschecks the output of theexecutemethod for a read-only function. The test correctly asserts the expected output, including the command to set the function attributes. The use ofPRINT_CONTEXTis consistent with the changes made to thePrintFunctionsstruct.236-241: The test
selecting_readonly_functionschecks the output of theexecutemethod when filtering for read-only functions. The test correctly asserts the expected output, including the command to set the function attributes. The use ofPRINT_CONTEXTis consistent with the changes made to thePrintFunctionsstruct.261-265: The test
selecting_non_readonly_functionschecks the output of theexecutemethod when filtering for non-read-only functions. The test correctly asserts the expected output, which should only include the non-read-only function.275-283: The test
function_not_foundchecks the error handling of theexecutemethod when the specified functions are not found in theFunctionSet. The test correctly asserts the expected errors, which are instances ofExecuteError::PrintUnsetFunction.287-393: The tests in the
non_default_contextmodule verify the behavior of theexecutemethod with differentPrintContextconfigurations. The tests correctly assert the expected outputs based on the context settings, such as the significance of the built-in name and the allowed options. These tests are important to ensure that thePrintContextstruct is being used correctly within thePrintFunctionsstruct.yash-builtin/src/typeset/print_variables.rs (24)
23-29: The
executefunction inPrintVariablesreturns aResult<String, Vec<ExecuteError>>, which is good for error handling. However, it's important to ensure that all callers of this function are updated to handle the new return type if this is a change from the previous implementation.57-63: The
print_onefunction has been updated to use the newPrintContext. Ensure that all calls to this function have been updated to pass the newPrintContextinstead of the previousPrintVariablesContext.89-95: The logic for printing array variables has been updated to consider
builtin_is_significant. Ensure that the new behavior is consistent with the expected output format for array variables in all use cases.151-157: The test
printing_one_variableuses a hardcodedPRINT_CONTEXT. IfPRINT_CONTEXThas been updated or replaced in the codebase, this test should be updated to reflect those changes.174-180: Similar to the previous comment, the test
printing_multiple_variablesshould be verified to use the correct context if there have been changes toPRINT_CONTEXT.193-198: The test
printing_array_variableshould be verified for the correct usage ofPRINT_CONTEXT, especially since array printing logic has been updated in the main code.207-212: The test
printing_valueless_variableshould be verified for the correct usage ofPRINT_CONTEXT.228-234: The test
quoting_variable_names_and_valuesshould be verified for the correct usage ofPRINT_CONTEXT.254-260: The test
printing_global_and_local_variables_at_onceshould be verified for the correct usage ofPRINT_CONTEXT.278-284: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [278-292]
The tests
printing_local_variables_onlyand the following test for printing a variable that is not found in the local scope should be verified for the correct usage ofPRINT_CONTEXT.
315-321: The test
printing_all_global_and_local_variablesshould be verified for the correct usage ofPRINT_CONTEXT.346-352: The test
printing_all_local_variablesshould be verified for the correct usage ofPRINT_CONTEXT.370-376: The test
printing_attributes_of_valueless_variablesshould be verified for the correct usage ofPRINT_CONTEXT.397-403: The test
printing_attributes_of_scalar_variablesshould be verified for the correct usage ofPRINT_CONTEXT.424-430: The test
printing_attributes_of_array_variablesshould be verified for the correct usage ofPRINT_CONTEXT.457-462: The test
selecting_readonly_variablesshould be verified for the correct usage ofPRINT_CONTEXT.473-478: The test
selecting_non_readedonly_variablesshould be verified for the correct usage ofPRINT_CONTEXT.489-494: The test
selecting_exported_variablesshould be verified for the correct usage ofPRINT_CONTEXT.505-510: The test
selecting_non_exported_variablesshould be verified for the correct usage ofPRINT_CONTEXT.520-524: The test
selecting_with_multiple_filtering_attributesshould be verified for the correct usage ofPRINT_CONTEXT.534-541: The test
variable_not_foundshould be verified for the correct usage ofPRINT_CONTEXT.562-570: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [562-577]
The test
builtin_namein thenon_default_contextmodule should be verified for the correct usage ofPRINT_CONTEXTand to ensure that the newbuiltin_namefield is being used correctly.
581-607: The tests
builtin_is_significantandinsignificant_builtin_with_attributed_arrayshould be verified for the correct usage ofPRINT_CONTEXTand to ensure that the newbuiltin_is_significantfield is being used correctly.650-658: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [650-682]
The tests in the
options_allowedsection should be verified for the correct usage ofPRINT_CONTEXTand to ensure that the newoptions_allowedfield is being used correctly.
magicant
force-pushed
the
readonly-builtin
branch
from
16929dc to
8a2d50a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (9)
- yash-builtin/src/export.rs (5 hunks)
- yash-builtin/src/lib.rs (1 hunks)
- yash-builtin/src/readonly.rs (2 hunks)
- yash-builtin/src/typeset.rs (10 hunks)
- yash-builtin/src/typeset/print_functions.rs (12 hunks)
- yash-builtin/src/typeset/print_variables.rs (27 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/readonly-p.sh (1 hunks)
- yash/tests/scripted_test/typeset-y.sh (2 hunks)
Files skipped from review due to trivial changes (3)
- yash-builtin/src/lib.rs
- yash/tests/scripted_test.rs
- yash/tests/scripted_test/typeset-y.sh
Additional comments: 55
yash-builtin/src/readonly.rs (5)
60-60: The TODO comment about linking to the eval built-in should be addressed if the documentation is ready to be linked.
129-129: Similar to the previous comment, the TODO about linking to the eval built-in should be addressed if the documentation is ready to be linked.
220-220: The TODO comment about splitting into syntax and semantics submodules should be removed if this refactoring has been completed, or it should be updated to reflect the current state of the codebase.
223-248: The
mainfunction has been correctly updated to be asynchronous and return aResultwrapped in a future. The use ofparse,interpret, andexecutefunctions for handling options, operands, and command execution is a good refactor for maintainability and readability.262-268: The test case
builtin_defines_read_only_variablehas been updated to reflect the new asynchronous nature of themainfunction. The use ofnow_or_never()fromfutures_utilto immediately resolve the future is appropriate for testing.yash-builtin/src/export.rs (5)
26-33: The updated documentation clearly explains the behavior of the
exportbuilt-in with and without the-poption. This aligns with the summary of changes indicating clarification in the documentation.43-61: The documentation now includes a description of the operands and the standard output behavior, which is consistent with the summary of changes. It is important to ensure that the behavior described here matches the actual implementation in the code.
80-87: The implementation notes have been updated to reflect the relationship between the
exportandtypesetbuilt-ins. This should be verified to ensure that the behavior described is consistent with the actual code.95-113: The
PRINT_VARIABLES_CONTEXThas been renamed toPRINT_CONTEXTand its type updated toPrintContext, as mentioned in the summary. This change should be cross-checked with other parts of the codebase to ensure consistency.129-134: The
mainfunction now handles theCommand::SetFunctionsandCommand::PrintFunctionscases withunreachable!macros, which is a good way to assert conditions that are expected to never happen. However, it is crucial to ensure that these cases are indeed not possible in the current context of theexportbuilt-in.yash-builtin/src/typeset.rs (9)
82-86: The TODO comment about linking to the eval built-in should be addressed or removed if it is no longer relevant.
163-167: The note about the built-in operating on existing shell functions only is important. It should be made clear in the documentation that this built-in cannot be used to create new functions or modify the body of existing functions.
227-234: The error conditions are well documented, which is good for maintainability and understanding the limitations of the built-in's functionality.
263-270: The explanation of how the implementation of the typeset built-in is shared with the
exportandreadonlybuilt-ins is clear and provides good insight into the code's modularity.354-355: The
Scopeenum is well-defined, making it clear how the scope of operations is determined for printing variables.393-394: The
PrintFunctionsstruct is well-defined, making it clear how the attributes for printing functions are determined.402-428: The
PrintContextstruct is a good addition for controlling the details of the commands when printing variables or functions. It allows for flexibility and reusability of the printing logic.482-493: The
executemethod is well-structured and handles the different command types effectively. It's good to see that it returns aResulttype, which is idiomatic Rust for error handling.646-652: The
mainfunction is nowasyncand properly handles the execution of the typeset built-in, including error reporting. This aligns with the asynchronous nature of the shell environment.yash-builtin/src/typeset/print_functions.rs (13)
22-30: The
executemethod in thePrintFunctionsstruct has been updated to take acontextparameter of type&PrintContext. This change aligns with the overall refactoring effort to parameterize the implementation and control the details of the commands using thePrintContextstruct.34-35: The comment indicates a TODO for honoring the collation order in the locale. This is a known enhancement that has not been implemented due to the extensive changes required across the codebase. It's important to track this as a potential future improvement.
56-60: The
print_onefunction now takes an extra parametercontextof type&PrintContext. This change is part of the refactoring to pass additional context information to the printing functions.67-96: The
print_onefunction has been updated to include logic for printing function attributes based on thePrintContext. This includes handling thebuiltin_is_significantflag and printing options allowed by the context. This change enhances the flexibility and configurability of the printing behavior.123-127: A test case has been added to verify the printing of a single function. This test ensures that the
executemethod correctly prints the function body when provided with a specific function name.147-152: Another test case has been added to verify the printing of multiple functions. This test checks that the
executemethod correctly prints the bodies of multiple functions in the order they are provided.173-178: This test case verifies that all functions are printed and that the result is sorted by function name. This aligns with the new behavior of printing shell variables and functions in alphabetical order.
192-197: The test case for quoting function names ensures that function names that require quoting are correctly prefixed with the
functionkeyword and quoted in the output.211-216: The test case for printing read-only functions checks that the
executemethod correctly prints the function body and the command to set the function as read-only.236-241: This test case verifies the selection and printing of read-only functions, ensuring that the
executemethod correctly filters and prints functions based on their attributes.261-266: The test case for selecting non-read-only functions ensures that the
executemethod correctly filters out read-only functions when the attribute filter specifies non-read-only functions.275-284: The test case for handling functions not found in the
FunctionSetchecks that theexecutemethod correctly returns errors for each function that does not exist.290-393: A series of test cases have been added under the
non_default_contextmodule to cover different scenarios using thePrintContextstruct. These tests verify the behavior of theexecutemethod when thebuiltin_name,builtin_is_significant, andoptions_allowedfields of thePrintContextare varied.yash-builtin/src/typeset/print_variables.rs (23)
23-29: The
executemethod has been updated to use the newPrintContexttype. However, the method signature change fromPrintVariablesContexttoPrintContextmay affect other parts of the code that rely on this method. Ensure that all calls toexecutehave been updated accordingly.57-63: The
print_onefunction has been updated to use the newPrintContexttype. This change is consistent with the refactoring mentioned in the summary. Ensure that all calls toprint_onehave been updated to pass the newPrintContexttype.89-95: The logic for printing array variables has been updated to include a check for
builtin_is_significant. This is a new field in thePrintContextstruct and affects the output format. Ensure that the new behavior is intended and correctly implemented across all use cases.151-157: The test case
printing_one_variablehas been updated to use the newPrintContextinstead of the oldPRINT_VARIABLES_CONTEXT. This change is consistent with the refactoring mentioned in the summary. Ensure that the test case still passes with the new context.174-180: The test case
printing_multiple_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.193-198: The test case
printing_array_variablehas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.207-212: The test case
printing_valueless_variablehas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.228-234: The test case
quoting_variable_names_and_valueshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.254-260: The test case
printing_global_and_local_variables_at_oncehas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.278-284: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [278-293]
The test cases for printing local variables only and attempting to print a global variable in the local scope have been updated to use the new
PrintContext. Ensure that the test cases still pass with the new context and that the output format is as expected, including the error case.
315-321: The test case
printing_all_global_and_local_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.346-352: The test case
printing_all_local_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.370-376: The test case
printing_attributes_of_valueless_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.397-403: The test case
printing_attributes_of_scalar_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.424-430: The test case
printing_attributes_of_array_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.457-462: The test case
selecting_readonly_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.473-478: The test case
selecting_non_readonly_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.489-494: The test case
selecting_exported_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.505-510: The test case
selecting_non_exported_variableshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.520-524: The test case
selecting_with_multiple_filtering_attributeshas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the output format is as expected.534-541: The test case
variable_not_foundhas been updated to use the newPrintContext. Ensure that the test case still passes with the new context and that the error handling is correct.562-570: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [562-577]
The test cases in the
non_default_contextmodule have been updated to use the newPrintContext. Ensure that the test cases still pass with the new context and that the output format is as expected, including the handling of thebuiltin_nameandbuiltin_is_significantfields.
- 650-658: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [650-682]
The test cases for
options_allowedhave been updated to use the newPrintContext. Ensure that the test cases still pass with the new context and that the output format is as expected, including the handling of different option specifications.
The previous tests contained redundant commands that were not run as a consequence of the preceding assignment failing. This patch removes those redundant commands and adds comments to explain the expected behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- yash-builtin/src/typeset.rs (10 hunks)
- yash/tests/scripted_test/readonly-p.sh (1 hunks)
Additional comments: 8
yash-builtin/src/typeset.rs (8)
82-86: The TODO comment about linking to the eval built-in should be addressed or removed if it is no longer relevant.
124-145: The explanation and example provided for the standard output when printing variables, especially array variables, is clear and informative. This aligns with the previous learning about adding an example of the output format for clarity.
174-175: The clarification that it is an error to modify a non-existing function is important for preventing undefined behavior and should be clearly documented.
236-242: The error conditions are well documented, which is crucial for users to understand the limitations and expected behavior of the built-in commands.
270-278: The modification of the implementation notes to indicate that the implementation of the typeset built-in is also used by the
exportandreadonlybuilt-ins is a good practice for maintainability and understanding of the codebase.363-368: The
Scopeenum is well-defined, providing clarity on the behavior of the typeset built-in in different contexts.402-407: The
PrintFunctionsstruct is well-defined, providing clarity on the behavior of the typeset built-in when printing functions.491-503: The
executefunction is well-implemented, handling different commands appropriately and returning aResulttype which is a good practice for error handling.
Summary by CodeRabbit
New Features
exportandreadonlybuilt-in commands with clarified behavior and additional documentation.typesetbuilt-in to print variables and functions in alphabetical order and handle array variables more effectively.Documentation
readonlybuilt-in covering behavior, options, operands, and standard output.Refactor
PRINT_VARIABLES_CONTEXTtoPRINT_CONTEXTacross various modules for consistency.typesetbuilt-in implementation to support different behaviors and introducedPrintContextfor command details.Tests
readonlyandtypesetbuilt-ins to ensure compliance with POSIX standards.readonly-p.shand updatedtypeset-y.shtest scripts for comprehensive testing of shell built-ins.Bug Fixes
readonlybuilt-in to prevent re-assignment of read-only variables.