Bugzilla – Full Text Bug Listing
| Summary: | [quick fix] to fix reliance on default encoding | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Carsten Hammer <carsten.hammer> |
| Component: | UI | Assignee: | JDT-UI-Inbox <jdt-ui-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P5 | CC: | jkubitz-eclipse, loskutov |
| Version: | 4.20 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/179119 | ||
| Whiteboard: | |||
|
Description
Carsten Hammer
Please mention if there is a bug due to the current code using the default encoding. Otherwise, I don't see a need to make changes. (In reply to Noopur Gupta from comment #1) > Please mention if there is a bug due to the current code using the default > encoding. Otherwise, I don't see a need to make changes. ok, there is Bug 566570 but I think we can leave out dogfoodings for this quickfix. This issue was meant to be about the quickfix implementation. I just tried to justify it. You can see a wip prototype that works for some parts attached to this bugzilla. Or do you think such a quickfix is in general not wanted? In that case I would like to know so that I stop working on it. (In reply to Carsten Hammer from comment #2) > Or do you think such a quickfix is in general not wanted? In that case I > would like to know so that I stop working on it. We can leave this bug open with low priority to see if other users come up with this request in general. If so, you can resume your work on it after that. (In reply to Noopur Gupta from comment #3) > (In reply to Carsten Hammer from comment #2) > > Or do you think such a quickfix is in general not wanted? In that case I > > would like to know so that I stop working on it. > > We can leave this bug open with low priority to see if other users come up > with this request in general. If so, you can resume your work on it after > that. Ok, if you can share some insight why using the default encoding is not a constant source of problems that would be great. And why the spotbugs errors regarding this are invalid. We have had a lot of problems especially with eclipse while we have been changing from windows to linux and back very often in projects. In parts it was because of svn support in eclipse but that does not explain everything. I would have been in need for some help to finish the gerrit anyway - so no problem if that is not wanted. I just want to understand your opinion as it is a kind of surprise for me. Neither the spotbugs errors nor the JEP400 https://openjdk.java.net/jeps/400 suggest that this problem is a kind of "no issue"... I believe, what Noopur meant was: do we have carefully evaluated if the code in question should use UTF8 and not default platform encoding? May be it uses default platform encoding *on purpose*? Applying quickfix blindly is a no-go here, the change should have been understood by someone who undertands the code in question, or we should have a bug report related to the code in question. See especially dicussion on bug 516583 and if you want help, feel free to work on bug 479450. (In reply to Andrey Loskutov from comment #5) > I believe, what Noopur meant was: do we have carefully evaluated if the code > in question should use UTF8 and not default platform encoding? May be it > uses default platform encoding *on purpose*? Applying quickfix blindly is a > no-go here, the change should have been understood by someone who undertands > the code in question, or we should have a bug report related to the code in > question. > > See especially dicussion on bug 516583 and if you want help, feel free to > work on bug 479450. Thanks for sharing your thoughts both of you. @Andrey, I guess you did not have a look at the gerrit and have been confused by my try to explain why a quickfix for that makes sense: This issue is not about applying a quickfix blindly - it is about *creating* a quickfix that is able to find and change code that implicitly makes use of the default encoding. Just try the attached wip quickfix - although it is broken in this state you get the idea what it should do - in some cases it is already working somehow. (In reply to Noopur Gupta from comment #3) > (In reply to Carsten Hammer from comment #2) > > Or do you think such a quickfix is in general not wanted? In that case I > > would like to know so that I stop working on it. > > We can leave this bug open with low priority to see if other users come up > with this request in general. If so, you can resume your work on it after > that. So far nobody else has been interested. So I gonna abandon it. Before I abandon the quickfix - would you mind to elaborate on why it is a bad idea? I am still curious to understand that. replacing for example String.getBytes() with static final Charset chset= Charset.defaultCharset(); String.getBytes(chset) is not a fix but just an inlining and doesnt help. Further beeing able to override the defaultCharset with a another charset is not a fix in itself but may even introduce errors where defaultCharset was meant. I renember some usage where defaultCharset was just used as fallback if the user supplied charset was invalid (One can define the charset to be used for files on file level). It might be good to evaluate each single usage of defaultCharset and add a comment why it is used. May be it turns out some of those defaultCharset usages are wrong - but is has to bee shown for each individual usage. Sure everybody would love if a bug gets fixed - if it is a bug. This cannot be checked automatically so i don't see a possibleautomatic cleanup. What could be automated is to add a warning comment of defaultCharset - but thats findbugs job?! (In reply to Jörg Kubitz from comment #8) > replacing for example > String.getBytes() > with > static final Charset chset= Charset.defaultCharset(); > String.getBytes(chset) > > is not a fix but just an inlining and doesnt help. > > Further beeing able to override the defaultCharset with a another charset is > not a fix in itself but may even introduce errors where defaultCharset was > meant. > I renember some usage where defaultCharset was just used as fallback if the > user supplied charset was invalid (One can define the charset to be used for > files on file level). > > It might be good to evaluate each single usage of defaultCharset and add a > comment why it is used. May be it turns out some of those defaultCharset > usages are wrong - but is has to bee shown for each individual usage. Sure > everybody would love if a bug gets fixed - if it is a bug. This cannot be > checked automatically so i don't see a possibleautomatic cleanup. What could > be automated is to add a warning comment of defaultCharset - but thats > findbugs job?! That's right, that is not a fix. Because of that the quickfix should have at least 3 different refactoring levels. One level does not change anything, the other ones change the charset to UTF-8 like what will happen in JEPS 400 or aggregate all usages to constants that can be easily tracked and changed afterwards. See https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/179119/14/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/preferences/cleanup/JavaFeatureTabPage.java how this is implemented. (In reply to Carsten Hammer from comment #9) > That's right, that is not a fix. Because of that the quickfix should have at > least 3 different refactoring levels. One level does not change anything, > the other ones change the charset to UTF-8 like what will happen in JEPS 400 > or aggregate all usages to constants that can be easily tracked and changed > afterwards. So step 1 on its own is useless and step 2 and 3 are DANGEROUS because they can introduce errors. That may intorduce a lot of errors on windows. It would destroy my name which uses o umlaut. I guess jep 400 wont make it for the same reason. (In reply to Jörg Kubitz from comment #10) > (In reply to Carsten Hammer from comment #9) > > > That's right, that is not a fix. Because of that the quickfix should have at > > least 3 different refactoring levels. One level does not change anything, > > the other ones change the charset to UTF-8 like what will happen in JEPS 400 > > or aggregate all usages to constants that can be easily tracked and changed > > afterwards. > > So step 1 on its own is useless and step 2 and 3 are DANGEROUS because they > can introduce errors. That may intorduce a lot of errors on windows. It > would destroy my name which uses o umlaut. I guess jep 400 wont make it for > the same reason. No, if you exchange the use of implict encding by constant you can set the constant to the same value implicit encoding would have choosen and have no change and by changing a single constant you can change behavior. This is a big help for a skilled programmer because the cleanup can find the code that is involved in implicit encoding much easier than you can do without a tooling. Btw. the errors are already there even in eclipse. (In reply to Carsten Hammer from comment #11) > by changing a single constant you can change behavior. Thats the problem: you change the behavior of all usages at once. Whereas only some of them are a real problem and others will silently break; > This is a big help for a skilled programmer because the cleanup can find the Now its getting odd: "a skilled programmer" who uses defaultEncoding ;-) Carsten, i know we can have different opinions and may have different environments. Might be somewhere exists code that needs exactly such a cleanup - But i do not know any codebase where such an automatic cleanup will not introduce errors. A warning is a good thing but it needs hard work to investigate whether it is a false positive. (In reply to Jörg Kubitz from comment #12) > (In reply to Carsten Hammer from comment #11) > > > by changing a single constant you can change behavior. > > Thats the problem: you change the behavior of all usages at once. Whereas > only some of them are a real problem and others will silently break; > > > This is a big help for a skilled programmer because the cleanup can find the > > Now its getting odd: "a skilled programmer" who uses defaultEncoding ;-) hidden default encoding is what should be resolved. So until you resolve it you need a simple way to investigate. The existing code with implicit default encoding is a bad base for such investigation. A quickfix often is just a step in a half automatic or half manually processs. It is a chance to promote type safe programming like Charset instead of String and so on - and to make visible what you are doing there. Obviously in the last 20 years it was a problem to resolve the issues on the eclipse codebase with the current approach. Otherwise they would not be there any longer. > > Carsten, i know we can have different opinions and may have different > environments. Might be somewhere exists code that needs exactly such a > cleanup - But i do not know any codebase where such an automatic cleanup > will not introduce errors. > A warning is a good thing but it needs hard work to investigate whether it > is a false positive. Yes, thats true - it needs work. I have done that a number of times already manually on codebases that runs on several platforms. Many cleanups are not supposed to be automatic in that you can apply without checking results. This one would be something to be checked. Let's give it up. I would have needed some help anyway and everybody here seems to be against that. Thanks all of you, it's clear now that it will not find support. Best regards, Carsten (In reply to Jörg Kubitz from comment #10) > (In reply to Carsten Hammer from comment #9) > > > That's right, that is not a fix. Because of that the quickfix should have at > > least 3 different refactoring levels. One level does not change anything, > > the other ones change the charset to UTF-8 like what will happen in JEPS 400 > > or aggregate all usages to constants that can be easily tracked and changed > > afterwards. > > So step 1 on its own is useless and step 2 and 3 are DANGEROUS because they > can introduce errors. That may intorduce a lot of errors on windows. It > would destroy my name which uses o umlaut. I guess jep 400 wont make it for > the same reason. Btw, jep400 is finally integrated and part of java 18 See https://openjdk.java.net/jeps/400 |