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

Fix case *_ error #4280

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix case *_ error #4280

wants to merge 2 commits into from

Conversation

Willie169
Copy link

Fix that case [a, *_] if a == 0: throws error rule soft_kw__not__wildcard failed predicate: {this.isnotEqualToCurrentTokenText("_")}?

@Willie169
Copy link
Author

Wait a minute. I forgot to test targets other than Java and add test for the fixed error.

Copy link
Contributor

@kaby76 kaby76 left a comment

Choose a reason for hiding this comment

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

Please add a .py file to examples/ that tests the changed rules for this PR.

@kaby76
Copy link
Contributor

kaby76 commented Oct 12, 2024

None of the examples/*.py tested as_pattern, closed_pattern, star_pattern, or double_star_pattern before. Yes, please add these so I can test the ambiguity. (The grammar before your changes has ambiguities.).

@Willie169
Copy link
Author

Ok, I will write examples for them

@Willie169
Copy link
Author

Willie169 commented Oct 13, 2024

I've added examples from Python 3.12 Standard Lib, examples written by me and ChatGPT, and Cpp target. I am not sure whether all targets work correctly.

@Willie169
Copy link
Author

test not passed: _bootstrap_external.py, _test_eintr.py, dump.py,test__interpchannels.py, test__interpreters.py,test_array.py, test_builtin.py, test_clinic.py,test_compile.py, test_exceptions.py, test_frame.py,test_fstring.py, test_imaplib.py, test_logging.py,test_opcache.py, test_os.py, test_posix.py,test_regrtest.py, test_str.py, test_subprocess.py,test_sys.py, test_tabnanny.py, test_traceback.py,test_type_params.py, test_venv.py

fix("PythonParser.g4");

return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rewrite this as a Python (v3) script. The reason is that the build currently checks for file transformGrammar.py (in ps1:

if (Test-Path -Path transformGrammar.py -PathType Leaf) {
$(& python3 transformGrammar.py ) 2>&1 | Write-Host
}
. The build does not know how to alter the grammar prior to running CMake. If it does not have a transformGrammar.py, then the grammar will not work in testing, and the Cpp target must be removed.

There are a multitude of examples in grammars-v4.

$ find . -name transformGrammar.py | grep Cpp
./bison/Cpp/transformGrammar.py
./cpp/Cpp/transformGrammar.py
./dart2/Cpp/transformGrammar.py
./fortran/fortran77/Cpp/transformGrammar.py
./fortran/fortran90/Cpp/transformGrammar.py
./golang/Cpp/transformGrammar.py
./gvpr/Cpp/transformGrammar.py
./javascript/javascript/Cpp/transformGrammar.py
./lua/Cpp/transformGrammar.py
./php/Cpp/transformGrammar.py
./python/python/Cpp/transformGrammar.py
./python/python3/Cpp/transformGrammar.py
./rust/Cpp/transformGrammar.py
./swift/swift5/Cpp/transformGrammar.py
10/13-09:29:45 ~/issues/g4-mysql-target-agnostic

Copy link
Contributor

@kaby76 kaby76 Oct 13, 2024

Choose a reason for hiding this comment

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

The alternative is to create "transformGrammar.py" which has Python compiling the .cpp file using g++, then running it. I think g++ is a dependency which is probably ok even on Windows. But, you will need to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

The final alternative that I can think of is to alter the templates st.build.ps1 and st.build.sh scripts (in _scripts/templates/Cpp and maybe the other targets) to perform the g++/run. Wish Antlr handled actions a little better.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I didn't noticed that, I think I will simply rewrite the transformGrammar with Python3.

@RobEin
Copy link
Contributor

RobEin commented Oct 14, 2024

Fix that case [a, *_] if a == 0: throws error rule soft_kw__not__wildcard failed predicate: {this.isnotEqualToCurrentTokenText("_")}?

I was able to reproduce the error.
This looks like a target independent bug.
I'm working on it.

@RobEin
Copy link
Contributor

RobEin commented Oct 14, 2024

Adding a CPP port is fine, although I am working on such a port as well.

The problem is that you are trying to overwrite the newer one with an older version, which causes a regression in many files.
You want to overwrite Python 3.12.6 with an older Python 3.12.1.
e.g. README.md

I don't see where you fixed the rule soft_kw__not__wildcard failed predicate error.

Please explain the many changes in PythonParser.g4.

@Willie169
Copy link
Author

Willie169 commented Oct 14, 2024

The real change that solve the problem is:

star_pattern
    : STAR NAME;

Other changes are useless and you can change them back.
And I think what causes the error may be that after the token is recognized as star_pattern and then as pattern_capture_target, it can't return to star_pattern when finding out it's not soft_kw__not__wildcard.

The escape quotes character in f-string can't be recognized, to reproduce:

f"\"", f'\''

I haven't solved it yet.

Btw, I haven't checked whether the Cpp port works but I will be busy on exams since today's midterm and probably until the 2nd GSAT mock test, so feel free to take over it or write another one.

@kaby76
Copy link
Contributor

kaby76 commented Oct 15, 2024

Some fixes are in order.

  • Undo the deletion of the TypeScript target and re-add the 4.13.2 requirement in desc.xml. The TypeScript port can't work without the changes that happened with Antlr 4.13.2.
  • The examples/ directory needs to be reorganized with some structure: Python Standard Lib code should go into its own directory called examples/Lib; ChatGPT-generated code should go into its own directory called examples/ChatGPT.

@RobEin
Copy link
Contributor

RobEin commented Oct 15, 2024

Thanks for the feedback about the two errors.
I tested your modified star_pattern rule and it really fixes the "soft_kw__not__wildcard failed predicate" error:

star_pattern
 : '*' NAME;

I'll even look into your further simplifications on this.
Unfortunately, many problems can occur with semantic predicates.
I will try to remove it from the parser grammar and replace it in the lexer grammar in some way.

I am also working on the "escape quotes character in f-string" error:
f"\"", f'\''

@RobEin
Copy link
Contributor

RobEin commented Oct 17, 2024

Looks like I managed to find and fix the "escape quotes character in f-string" bug.
I'll test it for a few more days.

Please change only the things related to the error "soft_kw__not__wildcard failed predicate" in PythonParser.g4 in the new PR.
I did not use token names in the parser grammar so that it resembles the official python.peg grammar as much as possible, and I think that the grammar is also more readable.
Please make the new PR based on the latest grammars-v4 repo.

Take PythonLexerBase.cpp out of the current PR for the time being, on the one hand because you haven't tested it, on the other hand, because the Cpp runtime must be completed first.
For example, the following methods are missing from Lexer.cpp:

  • getModeStack
  • getMode
  • setMode

Once you have the runtime modification, you can test PythonLexerBase.cpp.
And you can only make a PR about PythonLexerBase.cpp if the modified Cpp runtime has already been published.
Please let me know if you can do it.

@Willie169
Copy link
Author

Willie169 commented Oct 18, 2024

I will make this PR only adding star_pattern and delete others for now. And I will work on the Cpp after the exam if you haven't started that then. Thanks. Btw, the PR will be a part of my Learning Portfolio, something mandatory and counted in scores to hand in to universities when applying to back here.

@Willie169 Willie169 reopened this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants