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 color in appearing in fully qualified names. #246

Merged
merged 5 commits into from Oct 5, 2021
Merged

Conversation

sampottinger
Copy link
Collaborator

Addressing #240, allow color to appear in fully qualified names like as part of import statements. It's not the cleanest solution (other primitive types could not appear here) but I don't see too many alternatives.

It's not the cleanest solution but I don't see too many alternatives. Modify grammar to explicitly allow the color literal to appear in fully qualified name.
@dzaima
Copy link
Contributor

dzaima commented Aug 11, 2021

Other primitive types aren't allowed to be part of import statements anyways. A similar problem is with var, which is a valid identifier everywhere except for types (which this grammar doesn't support either). The "proper" way would be for 'color' and 'var' to not be special tokens, but instead specially handled identifiers in types (and I guess in the color() function), but that's certainly much more complicated.

Edit: Does the grammar even need to know about var? Shouldn't it being passed through as any other identifier work just fine?

@benfry
Copy link
Owner

benfry commented Aug 11, 2021

Yeah, I don't think this is actually something we want to enable/provide a hack to make it work… Using a reserved word is a no-no in any language, and carving out separate behavior is a maintenance nightmare. It's a bummer, but I think the alternative is worse.

@sampottinger
Copy link
Collaborator Author

sampottinger commented Aug 11, 2021 via email

@benfry
Copy link
Owner

benfry commented Aug 13, 2021

I think unless it's possible to have int or other primitive types in a package name, we shouldn't carve out an exception in the grammar. (If that's confirmed, we can close this.) We've spent so much time tracking down little quirks whenever we do something nonstandard like this that I don't want to introduce another exception.

There are also a lot of alternatives:

  • Set preproc.color_datatype = false in preferences.txt
  • Use a .java tab for the import (which won't use the preprocessor)
  • Change the name of the package
  • Use a full-fledged development environment like IntelliJ or Eclipse

The bottom line is that color is a primitive data type, and just an alias for int, and should behave just like int does in Java. It exists because it's super weird to explain to beginners that int is actually a color (sometimes but not always), and there's a significant performance impact if all colors are defined using objects.

@jeremydouglass
Copy link
Contributor

jeremydouglass commented Aug 17, 2021

This also means the color part of java.awt will not be directly accessible, correct?

import java.awt.color.ColorSpace

https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/java/awt/color/package-summary.html

If import foo.color worked in Processing 3 and now will not, I do wonder how many libraries are impacted. I did a quick scan of ~130 repos that were locally cached from the Archive for Processing collection, and I did not find any import *.color examples -- the closest that I found was Color classes, e.g. in colorscheme: https://github.com/josephtaylor/ColorScheme/blob/master/src/main/java/jto/colorscheme/Color.java

That said, there are maybe 500 packages I could check in the index -- and that doesn't speak to the larger java ecosystem.

@jeremydouglass
Copy link
Contributor

p.s. I should have also mentioned -- micycle's example in the issue (micycle.pgs.color.*) from the PGS library was not found my scan because that library was published in May, and I hadn't scraped it yet.

@sampottinger
Copy link
Collaborator Author

Just point of info, no int isn't allowed in Java package names. https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

@jeremydouglass
Copy link
Contributor

There is no other package in Java 11 with a .color namespace other than java.awt.color.ColorSpace

The only other major library I found out there is in Apache Commons -- org.apache.commons.imaging.color.

Guava's packages do not have a .color namespace.

@sampottinger
Copy link
Collaborator Author

sampottinger commented Aug 28, 2021

Yeah I think I see your point @jeremydouglass…. Especially for awt.color.

I know it’s weird to carve out an exception for color but it is the only primitive with this issue and the grammar change doesn’t concern me too much. It shouldn’t get caught up in the preprocessing rewrites because of the context and there’s tests for preproc in particular that could help catch a regression. I do see how this could be confusing for a new developer so I’d still be comfortable with the carve out.

@jeremydouglass
Copy link
Contributor

jeremydouglass commented Aug 28, 2021

That sounds good to me. As it stands the consequences are:

  1. you can't use java.awt.color.ColorSpace
  2. you can't use org.apache.commons.imaging.color
  3. micycle would need to renamespace the color part of PGS to run on Processing 4
  4. there might be something out there we didn't find but probably nothing major still hard to say how much
  5. it may come up periodically in the future as a weird corner case for new Processing or Java libraries
  6. Edit you can't import toxi.color

Those aren't great, but they are relatively minor compared to what I feared we might find I'm not sure how to rate it in bad-ness. If there was a long list of legacy libraries that would be broken by this then I would be begging for a grammar carve-out to allow imports with color namespaces. As it is, I still think a grammar carve-out might still be nice, BUT this isn't a hill I'm trying to die on if Ben disagrees. ;)

@postspectacular
Copy link
Contributor

FWIW I just got informed about this issue by a toxiclibs user, i.e. that import toxi.color.*; doesn't work in v4 anymore, but did work in any other recent & previous version in the last 20 years...

As you know, I don't actively maintain the library anymore, but if the final answer really is as @benfry stated above, then I will try to cut a new release with breaking changes (i.e. the package name changed) and dozens of examples sketches updated...

IMVHO this issue shouldn't even be specifically about color, but usage of 'reserved' words within fully qualified names in general. Inarguably, there's a clear syntactic, semantic and practical difference between using a reserved word like color as standalone individual token (i.e. as actual keyword/type name) or the same word occurring in a syntactically correct manner as part of a fully qualified package name... These two usages cannot just be equated (not should they be) and I literally can't think of any other language/environment where these two distinct use cases are treated as equal and/or as proposed here. But again, am not here to register my opposition, just giving my 2 euro cents... ✌️

@dzaima
Copy link
Contributor

dzaima commented Sep 9, 2021

I literally can't think of any other language/environment where these two distinct use cases are treated as equal and/or as proposed here.

@postspectacular Java is one such language with var (e.g. var var = 2; defines an integer variable named var; IIRC there's some more stuff like this in Java). "context sensitive keywords" is the search term for this stuff. They're used out of necessity for backwards compatibility of course, so it's not a thing you should do, but it's a reasonable option if you do want the backwards compatibility. (or, in this case, compatibility with Java too)

@postspectacular
Copy link
Contributor

@dzaima It shows then, I haven't used Java for ~10 years... Could have sworn there wasn't such a restriction for package names (and those are the only ones which matter here), but I stand corrected. The var example you've given I'd have classified as reserved-word-used-as-individual-identifier (of course illegal), but IMHO v. different from a multipart, dot-separated package name...

Anyhow, the only other thing somewhat bugging me a little here is that after 2 decades (congrats, btw!) of established behavior in P5, this use of color et al is now being counted as a 'illegal' and the new behavior marked as 'working as intended'. I do kind of get it, however, this stuff is breaking hundreds of existing projects (not just my own, also lots of students/peers) which otherwise would still work and I'm now having to explain this (again) to users... I shall file it under the same category as when years ago P5 suddenly stopped auto-importing various java.* packages, triggering a flood of confused users and requiring outside effort (i.e. not P5 core team) to spend extra effort updating existing workshop examples/projects/documentation. I do get (some of) the reasoning, but IMHO those kind of changes should be communicated more clearly/prominently (e.g. via better error messages, maybe w/ a link to a GH issue), not just a sudden generic syntax error. Again, my 2 cents...

@benfry
Copy link
Owner

benfry commented Oct 3, 2021

Given the impact as outlined here #246 (comment) that changes the priorities quite a bit. If @sampottinger has a fix that's working without too much impact, it sounds like we should make use of that. Sam, can you verify that you're comfortable with the reliability of the fix?

@benfry benfry merged commit 64ee899 into master Oct 5, 2021
@benfry benfry deleted the color-in-import branch October 5, 2021 02:19
@benfry
Copy link
Owner

benfry commented Oct 5, 2021

Giving this a shot for 4.0 beta 2 for sake of compatibility. Fingers crossed…

benfry added a commit that referenced this pull request Oct 5, 2021
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

This pull request has been automatically locked. Pull requests that have been closed are automatically locked 30 days after the last comment.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants