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

color in imports shows up as an error in the editor in 4.0 beta 9 #521

Closed
benfry opened this issue Jul 31, 2022 · 13 comments · Fixed by #636
Closed

color in imports shows up as an error in the editor in 4.0 beta 9 #521

benfry opened this issue Jul 31, 2022 · 13 comments · Fixed by #636
Labels
Help Wanted We have very little time and would like some help Preprocessor

Comments

@benfry
Copy link
Owner

benfry commented Jul 31, 2022

Code still compiles and runs, but is tagged as an error while editing.

To reproduce, see what happens when importing the toxiclibs library.

Possibly related: #519

@benfry benfry added Help Wanted We have very little time and would like some help Preprocessor labels Jul 31, 2022
@benfry
Copy link
Owner Author

benfry commented Jul 31, 2022

Screen Shot 2022-07-31 at 5 49 17 PM

@bsapozhnikov
Copy link
Contributor

@benfry I'm new here so apologies for my ignorance - what's the best way for me to contribute to issues like this one?
I've cloned the repo and played around with this issue; it looks to me like it can be addressed by updating exitColorPrimitiveType in the tree listener to not do the "color" -> "int" replacement when inside an import declaration. I'd be happy to open a PR if that direction sounds reasonable?

@benfry
Copy link
Owner Author

benfry commented Jan 13, 2023

Thanks for looking into it! Paging @sampottinger here since he's been the one working on that part of the code most actively, and can better answer your question.

@sampottinger
Copy link
Collaborator

Hey all! Strange it's an editor error only. Just referencing #240.

@sampottinger
Copy link
Collaborator

See also #246

@bsapozhnikov
Copy link
Contributor

Yea I also thought that was strange. I did see the issue and fix you linked above and also saw locally that those changes to the .g4 file are important to the actual compilation working properly. But to fix the editor error locally, I needed to make the change described above with existColorPrimitiveType.
I'm very new to the code so this is just a shot in the dark, but it does look like the build and error-checker flows use different logic to preprocess and compile the code (e.g. the error-checker uses the PreprocService while JavaBuild uses the PdePreprocessor directly) so it's possible that they handle edits and/or imports slightly differently? I haven't traced out the code enough to get a better answer though.
Conceptually though it seems like we don't want to do the color -> int conversion inside import declarations, right? Since inside of an import we're not treating color as a primitive the way we do elsewhere.

@sampottinger
Copy link
Collaborator

sampottinger commented Jan 14, 2023

Hey @bsapozhnikov! Yes that's right though they should hit the same preprocessor eventually which is what is odd. The grammar definition itself should match the import declaration (we technically have a problem if it gets parsed as a color primitive). So, in theory, we shouldn't hit the exit method for the color primitive at all for imports in the current code. I'd like to take look to see what the parser is seeing there. It could be related to #519. Should check that there aren't any other issues.

@sampottinger
Copy link
Collaborator

sampottinger commented Jan 14, 2023

Thanks for your help @bsapozhnikov.

Cool so the code from the editor is getting in and out of the preprocessor correctly in that the ANTLR output program is always right even when run from the preprocessor server. However, I think we have an issue when we go to generate the EDT edits 😑. Even though it’s the same grammar…

I agree @bsapozhnikov about the exitColorPrimitiveType but I think this could trip up any use of a qualified name for example lib.sublib.color.method().

I’ll have a PR up in a moment. Thank you for your help!

@benfry
Copy link
Owner Author

benfry commented Jan 14, 2023

Great! Incorporated for 4.1.2 or 4.2, whichever comes first.

@bsapozhnikov
Copy link
Contributor

Cool! Also a general question about contributing - if I dig into any other issues, should I be opening PRs or just continuing to post in the discussion?

@benfry
Copy link
Owner Author

benfry commented Jan 15, 2023

Probably worth checking in w/ the discussion in case it's something that's in progress, or no longer relevant, or just to make sure the problem is clear. This is where most PRs get tripped up, and hopefully we can prevent any wasted time.

Thanks for asking!

@sampottinger
Copy link
Collaborator

Thanks for participating in the community @bsapozhnikov!

@github-actions
Copy link

This issue has been automatically locked. To avoid confusion with reports that have already been resolved, closed issues are automatically locked 30 days after the last comment. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Help Wanted We have very little time and would like some help Preprocessor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants