Skip to content

Conversation

@carstenartur
Copy link
Contributor

@carstenartur carstenartur commented May 9, 2025

#1631

What it does

Port of "explicit encoding" cleanup from https://github.com/carstenartur/sandbox to eclipse

What it does

Some java api has been designed originally without a strong focus on safety on usage of different charsets. Because of that apis have simple string parameters to determine the encoding to use or even a default encoding that is based on the environment settings. The situation has improved a lot and you should explicitly set the charset. In doing so for type safety better switch to Charset class.

For an example what the result looks similar to look at
58ae341

That is what this cleanup is trying to support.

Starting with Java 10

1. 

ByteArrayOutputStream ba=new ByteArrayOutputStream();
String result=ba.toString();

Case 1: Option "keep_behavior"

ByteArrayOutputStream ba=new ByteArrayOutputStream();
String result=ba.toString(Charset.defaultCharset());

Case 2: Option "enforce_utf8"

ByteArrayOutputStream ba=new ByteArrayOutputStream();
String result=ba.toString(StandardCharsets.UTF_8);

2.

ByteArrayOutputStream ba=new ByteArrayOutputStream();
String result=ba.toString("UTF-8");

ByteArrayOutputStream ba=new ByteArrayOutputStream();
String result=ba.toString(StandardCharsets.UTF_8);

Starting with Java 10

Reader r=Channels.newReader(ch,"UTF-8");
Reader r=Channels.newReader(ch,StandardCharsets.UTF_8);

Starting with Java 10

Channels.newWriter(ch,"UTF-8");
Channels.newWriter(ch,StandardCharsets.UTF_8);

Starting with Java 18
Currently, jdt.ui supports test environments only up to Java 17, which means tests cannot be executed for this version.

1.

Charset.forName("UTF-8");
StandardCharsets.UTF_8;

2.

Charset.forName("UTF-16");
StandardCharsets.UTF_16;

Starting with Java 5

1.

new java.util.Formatter(new File(), "UTF-8");
new java.util.Formatter(new File(), StandardCharsets.UTF_8);

2.

new java.util.Formatter(new File(), "UTF-8",new java.util.Locale());
new java.util.Formatter(new File(), StandardCharsets.UTF_8,new java.util.Locale());

...

Starting with Java 5

new InputStreamReader(InputStream in, "UTF-8");
new InputStreamReader(InputStream in, StandardCharsets.UTF_8);

Starting with Java 5

new OutputStreamWriter os=new OutputStreamWriter(InputStream in,"UTF-8");
new OutputStreamWriter os=new OutputStreamWriter(InputStream in, StandardCharsets.UTF_8);

Starting with Java 5

Stream fw=new PrintStream("file.txt", "UTF-8");
Stream fw=new PrintStream("file.txt", StandardCharsets.UTF_8);

Starting with Java 10

Properties.storeToXML(java.io.OutputStream,"UTF-8");
Properties.storeToXML(java.io.OutputStream,StandardCharsets.UTF_8);

Starting with Java 10

new java.util.Scanner(new File("asdf"),"UTF-8");
new java.util.Scanner(new File("asdf"),StandardCharsets.UTF_8);

Starting with Java 10

String s=new String(byte[],"UTF-8");
String s=new String(byte[],StandardCharsets.UTF_8);

Starting with Java 5

String.getBytes("Utf-8")
String.getBytes(StandardCharsets.UTF_8);

Starting with Java 10

java.net.URLDecoder.decode("asdf","UTF-8");
java.net.URLDecoder.decode("asdf",StandardCharsets.UTF_8);

There are still some missing parts:

  1. Now using a Java 22 test environment since there is no Java 18 test enviroment support in jdt ui. Should be fine.
  2. In a number of cases the UnsupportedEncodingException is removed now. Several classes behave different. So there is no common approach to treat the situation.
  3. The code for PrintWriter,FileReader should be reimplemented in the way the other cleanups do it
  4. 3 different options are supported "KEEP" , "USE_UTF8" ,"USE_UTF8_AGGREGATE"
    KEEP means that the current behavior of the code is not changed
    USE_UTF-8 means that in all cases where there is no fixed encoding we add a charset UTF-8
    USE_UTF-8_AGGREGATE means the same as USE_UTF-8 but additionally add a constant to the class and refers to it
  5. NLS Comments are currently not removed
  6. This cleanup appears on the code style page although only using the first choice allows to keep the current behavior!

image

How to test

Author checklist

@carstenartur
Copy link
Contributor Author

carstenartur commented Jun 20, 2025

Talked about it in the "developers community call" on 19th of June, 2025.

#2196 (comment)

The difference between the i built that is causing issues here and the regular built which is not failing but is the only one a contributor can "reproduce" had been discussed.

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/wiki/Eclipse-IDE-%E2%80%90-Developers-Community-Call#19th-june-2025 Eclipse IDE ‐ Developers Community Call · eclipse-platform/eclipse.platform.releng.aggregator Wiki

See

eclipse-platform/eclipse.platform.releng.aggregator#2979 Streamline I-build test setup · Issue #2979 · eclipse-platform/eclipse.platform.releng.aggregator

To hightlight code that is inherently making use of environment settings regarding encoding one option is to simply use the api for marking words.

https://www.eclipse.org/articles/Article-Mark%20My%20Words/mark-my-words.html Mark My Words

An approach to make jdt core able to detect usage of such dependency to environment settings (which is in some situations what you want!)
should look at

https://github.com/eclipse-jdtls/eclipse-jdt-core-incubator Issues · eclipse-jdtls/eclipse-jdt-core-incubator

@HannesWell
Copy link
Contributor

The difference between the i built that is causing issues here and the regular built which is not failing but is the only one a contributor can "reproduce" had been discussed.

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/wiki/Eclipse-IDE-%E2%80%90-Developers-Community-Call#19th-june-2025 Eclipse IDE ‐ Developers Community Call · eclipse-platform/eclipse.platform.releng.aggregator Wiki

Yes, exactly. Just to mirror the notes: the difference is mainly the project 'shape' (in I-build test bundles are packed/zipped into jars, while in verification builds they are usually in a (not-zipped) directory structure directly on the file-system).

Unfortunately it's currently very hard to replicate the I-build locally, but my suggestion is to make sure that the tests don't expect any files being available in a usual (not-zipped) file-system folder structure. If possible only access resources through the class-loader or as bundle-entry. If you really need a directory structure on the file-system, you could extract the files from the project into a (JUnit) temporary-folder and replicate the structure there (maybe only if the project isn't already in a directory 'shape').
You could for example do it similar to:

@eclipse-jdt-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF
org.eclipse.jdt.ui.tests/META-INF/MANIFEST.MF
org.eclipse.jdt.ui.tests/pom.xml
org.eclipse.jdt.ui/META-INF/MANIFEST.MF
org.eclipse.jdt.ui/pom.xml

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 38e25cea918441e9069159c37dd45610c73cdaa7 Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <jdt-bot@eclipse.org>
Date: Mon, 1 Sep 2025 19:52:15 +0000
Subject: [PATCH] Version bump(s) for 4.38 stream


diff --git a/org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF b/org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF
index b7e371bfc0..2dc3fe58b1 100644
--- a/org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.core.manipulation
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.core.manipulation; singleton:=true
-Bundle-Version: 1.23.100.qualifier
+Bundle-Version: 1.23.200.qualifier
 Bundle-Vendor: %providerName
 Bundle-Activator: org.eclipse.jdt.internal.core.manipulation.JavaManipulationPlugin
 Bundle-Localization: plugin
diff --git a/org.eclipse.jdt.ui.tests/META-INF/MANIFEST.MF b/org.eclipse.jdt.ui.tests/META-INF/MANIFEST.MF
index 150de53e6e..945af6dd01 100644
--- a/org.eclipse.jdt.ui.tests/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.ui.tests/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.ui.tests
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.jdt.ui.tests; singleton:=true
-Bundle-Version: 3.16.200.qualifier
+Bundle-Version: 3.16.300.qualifier
 Bundle-Activator: org.eclipse.jdt.testplugin.JavaTestPlugin
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
diff --git a/org.eclipse.jdt.ui.tests/pom.xml b/org.eclipse.jdt.ui.tests/pom.xml
index 0bf5c821d5..dbfb4271c8 100644
--- a/org.eclipse.jdt.ui.tests/pom.xml
+++ b/org.eclipse.jdt.ui.tests/pom.xml
@@ -20,7 +20,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.ui.tests</artifactId>
-  <version>3.16.200-SNAPSHOT</version>
+  <version>3.16.300-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
   <properties>
     <defaultSigning-excludeInnerJars>true</defaultSigning-excludeInnerJars>
diff --git a/org.eclipse.jdt.ui/META-INF/MANIFEST.MF b/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
index 53f1b0d306..a5dc720767 100644
--- a/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.ui
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.ui; singleton:=true
-Bundle-Version: 3.35.100.qualifier
+Bundle-Version: 3.35.200.qualifier
 Bundle-Activator: org.eclipse.jdt.internal.ui.JavaPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %providerName
diff --git a/org.eclipse.jdt.ui/pom.xml b/org.eclipse.jdt.ui/pom.xml
index e81c09a125..fa432d58f4 100644
--- a/org.eclipse.jdt.ui/pom.xml
+++ b/org.eclipse.jdt.ui/pom.xml
@@ -18,7 +18,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.ui</artifactId>
-  <version>3.35.100-SNAPSHOT</version>
+  <version>3.35.200-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
 
 	<build>
-- 
2.51.0

Further information are available in Common Build Issues - Missing version increments.

@carstenartur carstenartur force-pushed the explicitencoding branch 2 times, most recently from 90d2e2b to 18e9f3c Compare September 6, 2025 15:38
@carstenartur carstenartur marked this pull request as ready for review October 4, 2025 10:16
@carstenartur
Copy link
Contributor Author

Unfortunately this one cannot be merged as is. Should anybody have time to have a look why it does not run through you are welcome.

carstenartur and others added 8 commits October 26, 2025 10:50
inherit specific processing in enum
Add more testcases and support aggregatedutf8 option
add option to remove unnecessary nls tags (not working)
- use ASTNodes.replaceAndRemoveNLS() instead of removeNLSComment()
- remove the AbstractExplicitEncoding.removeNLSComment() method
- remove aggregate option that uses constant
- fix some previews that are backwards
- fix description message to remove adding method names
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.

4 participants