Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[osh] Implement ${a[@]@P} and make ${a[@]@a} and ${a[@]@Q} consistent with Bash #2210

Merged
merged 7 commits into from
Jan 2, 2025

Conversation

akinomyoga
Copy link
Collaborator

This corresponds to PR(b) explained in #2201.

The behavior for @a

Others

This PR also includes the modification to @Q. Also, @P was only supported for a scalar variable, but now this PR implements it for BashArray/BashAssoc. 03cf713 is a related refactoring commit.

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

Thanks! It looks like there are a few type errors, which can be checked with

devtools/types.sh check-oils

See log at the end here:

http://op.oilshell.org/uuu/github-jobs/8824/cpp-coverage.wwz/_tmp/soil/logs/compare-gcc-clang.txt


Happy New Year!

osh/word_eval.py Outdated Show resolved Hide resolved
osh/prompt.py Outdated Show resolved Hide resolved
@akinomyoga akinomyoga force-pushed the array-var-op0 branch 2 times, most recently from 9634100 to 0a18208 Compare January 1, 2025 03:01
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Jan 1, 2025

I fixed type annotations in be89909..0a18208, and the type check passes in my local environment.

(Please ignore this. I found the description in mycpp/README.md)

However, I got the following C++ compilation error:

_gen/bin/oils_for_unix.mycpp.cc: In member function ‘BigStr* prompt::Evaluator::EvalFirstPrompt()’:
_gen/bin/oils_for_unix.mycpp.cc:41915:34: error: ‘class value_asdl::value_t’ has no member named ‘s’
41915 |     return this->EvalPrompt(val->s);
      |                                  ^

The corresponding Python code is this:

oils/osh/prompt.py

Lines 303 to 305 in 0a18208

if ps1_val.tag() == value_e.Str:
val = cast(value.Str, ps1_val)
return self.EvalPrompt(val.s)

Why is val still value_t even though I cast it to value.Str by val = cast(value.Str, ps1_val)?


edit: I found an answer in mycpp/README.md:

oils/mycpp/README.md

Lines 253 to 259 in f2c84da

Again, the names `UP_*` are **special**. If the name doesn't start with `UP_`,
the inner blocks will look like:
case value_e::Str: {
val = static_cast<value::Str>(val); // BAD: val reused
print(StrFormat(str43, val->s))
}


Happy New Year!

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Jan 1, 2025

(Resolved)

Why is val still value_t even though I cast it to value.Str by val = cast(value.Str, ps1_val)?

I thought you once told me that val = cast(...) would declare a new variable in the local scope so the type would be different from the outer scope, but it now seems to interfere with the type of val in the outer scope. If I change the variable name to val2, the problem doesn't happen. [ edit: The generated code looks like this

  ps1_val = this->mem->env_config->GetVal(S_Eni);
  if (ps1_val->tag() == value_e::Str) {
    val = static_cast<value::Str*>(ps1_val);
    return this->EvalPrompt(val->s);
  }
  else {
    return S_Aoo;
  }

where val is not declared to be a new variable. Does this mean that the behavior changes based on the presence of the corresponding UP_ variable? ]

Anyway, I decided to use the convention of UP_ps1_val here: 0a18208..c38fa23.

@akinomyoga
Copy link
Collaborator Author

I updated tests because now OSH implements the expected behavior: c38fa23..8e8271e.

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, a couple questions ....

a $'b\\nc'
status=0
a
a a a
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that OSH differs from bash because of a quoting difference, which is OK

But it also seems to differ with a a a vs. '' ?

Or maybe that is addressed in a follow-up?

TBH I don't understand the reasons for the evaluation logic - in these cases, my review falls back to "does OSH match bash?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! Thanks for pointing out this. This is just my oversight. OSH should generate the same result as Bash. Let me think about how we should distinguish array-origin BashArray from $@-origin BashArray.

Copy link
Contributor

@andychu andychu Jan 2, 2025

Choose a reason for hiding this comment

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

It is OK if we fix that later (or even not at all if nobody runs into it, and if bash does not document it)

Sometimes when I encounter a quirk like this, I split it into a separate test case, and then mark it oils_failures_allowed += 1

That is just a "note for later" (Basically because there seem to be infinite bash implementation details! I think using @ with a type query is EXTREMELY obscure. I will be surprised if anyone depends on this)

So that way the first test case have bash STDOUT === osh STDOUT, which is a nice proprety

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you wait for a while? If it turns out to be easy to fix, I can fix it within this PR. If not, I'll handle it in a separate PR later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to defer it. The best solution could be to introduce a new type to represent the list coming from $@, but one might relate it to the forthcoming switching from BashArray to SparseArray. We can discuss that later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now this is an allowed OSH failure: fda3cd4

spec/var-op-bash.test.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

OK sounds good! I made a comment about minimizing "OK bash" blocks when our intention is to agree with bash, but this is a matter of judgement

There will always be some of these, but IMO it is easier to maintain later if we don't have to read and edit the large assertion blocks, because it's easy to make mistakes, i.e. copy/paste bugs where we didn't actually fix things

spec/var-op-bash.test.sh Show resolved Hide resolved
@andychu andychu merged commit 1485501 into soil-staging Jan 2, 2025
18 checks passed
@akinomyoga akinomyoga deleted the array-var-op0 branch January 2, 2025 16:54
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