Skip to content

Commit

Permalink
Coercion of string "false" to boolean now provides a warning.
Browse files Browse the repository at this point in the history
This is usually not what the user meant to do, as string "false"
evaluates to boolean true. Therefore, when we try to coerce a string
into a boolean, we first check to see if it is the string "false" and
if so, issue a warning. Eventually, I would like this to be configurable
per file, or at least at a more granular level, but anyways, for now 
this is a globally configurable warning in the logger-preferences.ini
settings.

This also required a change to the Static.getBoolean method, which has
been updated throughout the codebase to add a Target parameter. 
However, because extensions may
be using this method, it remains backwards compatible for now, but
the no Target method is deprecated and should be removed at next major
version change.
  • Loading branch information
LadyCailin committed Mar 27, 2018
1 parent e17b850 commit 7dd4957
Show file tree
Hide file tree
Showing 40 changed files with 279 additions and 174 deletions.
5 changes: 5 additions & 0 deletions src/main/java/com/laytonsmith/core/ArgumentValidation.java
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,11 @@ public static boolean getBoolean(Construct c, Target t) {
if(c instanceof CBoolean) {
b = ((CBoolean) c).getBoolean();
} else if(c instanceof CString) {
if(((CString)c).val().equals("false")) {
CHLog.GetLogger().e(CHLog.Tags.FALSESTRING, "String \"false\" evaluates as true (non-empty strings are"
+ " true). This is most likely not what you meant to do. This warning can globally be disabled"
+ " with the logger-preferences.ini file.", t);
}
b = (c.val().length() > 0);
} else if(c instanceof CInt || c instanceof CDouble) {
b = !(getNumber(c, t) == 0);
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/com/laytonsmith/core/CHLog.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ private CHLog() {
public enum Tags {
COMPILER("compiler", "Logs compiler errors (but not runtime errors)", LogLevel.WARNING),
RUNTIME("runtime", "Logs runtime errors, (exceptions that bubble all the way to the top)", LogLevel.ERROR),
FALSESTRING("falsestring", "Logs coersion of the string \"false\" to boolean, which is actually true",
LogLevel.ERROR),
DEPRECATION("deprecation", "Shows deprecation warnings", LogLevel.WARNING),
PERSISTENCE("persistence", "Logs when any persistence actions occur.", LogLevel.ERROR),
//TODO Add the rest of these hooks into the code
Expand Down Expand Up @@ -115,7 +117,12 @@ private static LogLevel GetLevel(Tags tag) {
}
LogLevel level;
try {
level = LogLevel.valueOf((String) prefs.getPreference(tag.name));
String pref = (String) prefs.getPreference(tag.name);
if("ON".equals(pref)) {
level = LogLevel.ERROR;
} else {
level = LogLevel.valueOf(pref);
}
} catch(IllegalArgumentException e) {
level = LogLevel.ERROR;
}
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/com/laytonsmith/core/ObjectGenerator.java
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ public MCItemMeta itemMeta(Construct c, MCMaterial mat, Target t) throws ConfigR

if(Static.getServer().getMinecraftVersion().gte(MCVersion.MC1_11)) {
if(ma.containsKey("unbreakable")) {
meta.setUnbreakable(Static.getBoolean(ma.get("unbreakable", t)));
meta.setUnbreakable(Static.getBoolean(ma.get("unbreakable", t), t));
}
}
}
Expand Down Expand Up @@ -1040,10 +1040,10 @@ public List<MCLivingEntity.MCEffect> potions(CArray ea, Target t) {
}
}
if(effect.containsKey("ambient")) {
ambient = Static.getBoolean(effect.get("ambient", t));
ambient = Static.getBoolean(effect.get("ambient", t), t);
}
if(effect.containsKey("particles")) {
particles = Static.getBoolean(effect.get("particles", t));
particles = Static.getBoolean(effect.get("particles", t), t);
}
ret.add(new MCLivingEntity.MCEffect(potionID, strength, (int) (seconds * 20), ambient, particles));
} else {
Expand Down Expand Up @@ -1116,10 +1116,10 @@ public CArray fireworkEffect(MCFireworkEffect mcfe, Target t) {
public MCFireworkEffect fireworkEffect(CArray fe, Target t) {
MCFireworkBuilder builder = StaticLayer.GetConvertor().GetFireworkBuilder();
if(fe.containsKey("flicker")) {
builder.setFlicker(Static.getBoolean(fe.get("flicker", t)));
builder.setFlicker(Static.getBoolean(fe.get("flicker", t), t));
}
if(fe.containsKey("trail")) {
builder.setTrail(Static.getBoolean(fe.get("trail", t)));
builder.setTrail(Static.getBoolean(fe.get("trail", t), t));
}
if(fe.containsKey("colors")) {
Construct colors = fe.get("colors", t);
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/laytonsmith/core/Static.java
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,28 @@ public static byte getInt8(Construct c, Target t) {
* is empty, it is false, otherwise it is true.
*
* @param c
* @param t
* @return
*/
public static boolean getBoolean(Construct c, Target t) {
return ArgumentValidation.getBoolean(c, t);
}

/**
* Returns a boolean from any given construct. Depending on the type of the construct being converted, it follows
* the following rules: If it is an integer or a double, it is false if 0, true otherwise. If it is a string, if it
* is empty, it is false, otherwise it is true.
*
* @param c
* @return
* @deprecated Use
* {@link #getBoolean(com.laytonsmith.core.constructs.Construct, com.laytonsmith.core.constructs.Target)}
* instead, as it provides better error messages for users that use the string "false" as a boolean. This method
* should be removed in version 3.3.3 or above.
*/
@Deprecated
public static boolean getBoolean(Construct c) {
return ArgumentValidation.getBoolean(c, Target.UNKNOWN);
return getBoolean(c, Target.UNKNOWN);
}

/**
Expand Down
71 changes: 66 additions & 5 deletions src/main/java/com/laytonsmith/core/compiler/FileOptions.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
package com.laytonsmith.core.compiler;

import com.laytonsmith.PureUtilities.ClassLoading.ClassDiscovery;
import com.laytonsmith.PureUtilities.Version;
import com.laytonsmith.core.CHVersion;
import com.laytonsmith.core.Documentation;
import com.laytonsmith.core.Prefs;
import java.net.URL;
import java.util.ArrayList;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
*
Expand All @@ -12,10 +19,10 @@
public class FileOptions {

/*
These values are used in the syntax highlighter, and should remain the name they are in code.
These value names are used in the syntax highlighter, and should remain the name they are in code.
*/
private final Boolean strict;
private final List<String> suppressWarnings;
private final Set<SuppressWarnings> suppressWarnings;
private final String name;
private final String author;
private final String created;
Expand All @@ -24,7 +31,7 @@ public class FileOptions {

public FileOptions(Map<String, String> parsedOptions) {
strict = parseBoolean(getDefault(parsedOptions, "strict", null));
suppressWarnings = parseList(getDefault(parsedOptions, "suppresswarnings", ""));
suppressWarnings = parseEnumSet(getDefault(parsedOptions, "suppresswarnings", ""), SuppressWarnings.class);
name = getDefault(parsedOptions, "name", "");
author = getDefault(parsedOptions, "author", "");
created = getDefault(parsedOptions, "created", "");
Expand Down Expand Up @@ -55,6 +62,20 @@ private List<String> parseList(String list) {
}
return l;
}

private <T extends Enum<T>> Set<T> parseEnumSet(String list, Class<T> type) {
EnumSet<T> set = EnumSet.noneOf(type);
List<String> sList = parseList(list);
for(String s : sList) {
for(T e : type.getEnumConstants()) {
if(e.name().equals(s)) {
set.add(e);
break;
}
}
}
return set;
}

/**
* Returns whether or not this file is in strict mode. Unlike most options, this one depends on both the file
Expand All @@ -71,8 +92,8 @@ public boolean isStrict() {
}
}

public boolean isWarningSuppressed(String warning) {
return warning.trim().contains(warning.toLowerCase());
public boolean isWarningSuppressed(SuppressWarnings warning) {
return suppressWarnings.contains(warning);
}

public String getName() {
Expand Down Expand Up @@ -101,5 +122,45 @@ public String toString() {
+ (description == null ? "" : "File description: " + description + "\n");

}

public static enum SuppressWarnings implements Documentation {
// In the future, when some are added, this can be removed, and the rest of the system will work
// quite nicely. Perhaps a good first candidate would be to allow string "false" coerced to boolean warning
// to be suppressed on a per file basis?
Note("There are currently no warning suppressions defined, but some will be added in the future",
CHVersion.V0_0_0);

private SuppressWarnings(String docs, Version version) {
this.docs = docs;
this.version = version;
}

private final String docs;
private final Version version;
@Override
public URL getSourceJar() {
return ClassDiscovery.GetClassContainer(this.getClass());
}

@Override
public Class<? extends Documentation>[] seeAlso() {
return new Class[]{};
}

@Override
public String getName() {
return this.name();
}

@Override
public String docs() {
return docs;
}

@Override
public Version since() {
return version;
}
}

}
14 changes: 7 additions & 7 deletions src/main/java/com/laytonsmith/core/constructs/CArray.java
Original file line number Diff line number Diff line change
Expand Up @@ -876,11 +876,11 @@ public int compare(Construct o1, Construct o2) {
}
}
if(o1 instanceof CBoolean || o2 instanceof CBoolean) {
if(Static.getBoolean(o1) == Static.getBoolean(o2)) {
if(Static.getBoolean(o1, Target.UNKNOWN) == Static.getBoolean(o2, Target.UNKNOWN)) {
return 0;
} else {
int oo1 = Static.getBoolean(o1) ? 1 : 0;
int oo2 = Static.getBoolean(o2) ? 1 : 0;
int oo1 = Static.getBoolean(o1, Target.UNKNOWN) ? 1 : 0;
int oo2 = Static.getBoolean(o2, Target.UNKNOWN) ? 1 : 0;
return (oo1 < oo2) ? -1 : 1;
}
}
Expand All @@ -899,13 +899,13 @@ public int compare(Construct o1, Construct o2) {
}

public int compareRegular(Construct o1, Construct o2) {
if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o1))
&& Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o2))) {
if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o1), Target.UNKNOWN)
&& Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o2), Target.UNKNOWN)) {
return compareNumeric(o1, o2);
} else if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o1))) {
} else if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o1), Target.UNKNOWN)) {
//The first is a number, the second is a string
return -1;
} else if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o2))) {
} else if(Static.getBoolean(new DataHandling.is_numeric().exec(Target.UNKNOWN, null, o2), Target.UNKNOWN)) {
//The second is a number, the first is a string
return 1;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/laytonsmith/core/events/Prefilters.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ private static void ItemMatch(Construct item1, Construct item2) throws Prefilter
}

private static void BooleanMatch(Construct bool1, Construct bool2) throws PrefilterNonMatchException {
if(Static.getBoolean(bool1) != Static.getBoolean(bool2)) {
if(Static.getBoolean(bool1, Target.UNKNOWN) != Static.getBoolean(bool2, Target.UNKNOWN)) {
throw new PrefilterNonMatchException();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public boolean matches(Map<String, Construct> prefilter, BindableEvent e) throws
@Override
public BindableEvent convert(CArray manualObject, Target t) {
CmdlinePromptInput cpi = new CmdlinePromptInput(manualObject.get("command", t).val(),
Static.getBoolean(manualObject.get("shellMode", t)));
Static.getBoolean(manualObject.get("shellMode", t), t));
return cpi;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1300,10 +1300,10 @@ public boolean modifyEvent(String key, Construct value, BindableEvent event) {
e.setDeathMessage(value.nval());
return true;
case "keep_inventory":
e.setKeepInventory(Static.getBoolean(value));
e.setKeepInventory(Static.getBoolean(value, Target.UNKNOWN));
return true;
case "keep_level":
e.setKeepLevel(Static.getBoolean(value));
e.setKeepLevel(Static.getBoolean(value, Target.UNKNOWN));
return true;
case "new_exp":
e.setNewExp(Static.getInt32(value, Target.UNKNOWN));
Expand Down Expand Up @@ -2447,7 +2447,7 @@ public boolean modifyEvent(String key, Construct value, BindableEvent event) {
return true;
}
} else if(key.equalsIgnoreCase("signing")) {
((MCPlayerEditBookEvent) event).setSigning(Static.getBoolean(value));
((MCPlayerEditBookEvent) event).setSigning(Static.getBoolean(value, Target.UNKNOWN));
return true;
} else {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,11 @@ public boolean modifyEvent(String key, Construct value, BindableEvent event) {
if(event instanceof MCVehicleEnitityCollideEvent) {
MCVehicleEnitityCollideEvent e = (MCVehicleEnitityCollideEvent) event;
if(key.equals("collide")) {
e.setCollisionCancelled(!Static.getBoolean(value));
e.setCollisionCancelled(!Static.getBoolean(value, Target.UNKNOWN));
return true;
}
if(key.equals("pickup")) {
e.setPickupCancelled(!Static.getBoolean(value));
e.setPickupCancelled(!Static.getBoolean(value, Target.UNKNOWN));
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public static Reaction GetReaction(ConfigRuntimeException e, Environment env) {
if(ret instanceof CNull || Prefs.ScreamErrors()) {
reaction = Reaction.REPORT;
} else {
if(Static.getBoolean(ret)) {
if(Static.getBoolean(ret, Target.UNKNOWN)) {
reaction = Reaction.IGNORE;
} else {
reaction = Reaction.FATAL;
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/com/laytonsmith/core/functions/ArrayHandling.java
Original file line number Diff line number Diff line change
Expand Up @@ -2102,7 +2102,7 @@ public Construct exec(Target t, Environment environment, Construct... args) thro
throw new CRERangeException("Overflow detected. Number cannot be larger than " + Integer.MAX_VALUE, t);
}
if(args.length > 2) {
getKeys = Static.getBoolean(args[2]);
getKeys = Static.getBoolean(args[2], t);
}

LinkedHashSet<Integer> randoms = new LinkedHashSet<>();
Expand Down Expand Up @@ -2188,7 +2188,7 @@ public CArray exec(final Target t, final Environment environment, Construct... a
CArray array = Static.getArray(args[0], t);
boolean compareTypes = true;
if(args.length == 2) {
compareTypes = Static.getBoolean(args[1]);
compareTypes = Static.getBoolean(args[1], t);
}
final boolean fCompareTypes = compareTypes;
if(array.inAssociativeMode()) {
Expand All @@ -2200,8 +2200,8 @@ public CArray exec(final Target t, final Environment environment, Construct... a

@Override
public boolean checkIfEquals(Construct item1, Construct item2) {
return (fCompareTypes && Static.getBoolean(sequals.exec(t, environment, item1, item2)))
|| (!fCompareTypes && Static.getBoolean(equals.exec(t, environment, item1, item2)));
return (fCompareTypes && Static.getBoolean(sequals.exec(t, environment, item1, item2), t))
|| (!fCompareTypes && Static.getBoolean(equals.exec(t, environment, item1, item2), t));
}
});
for(Construct c : set) {
Expand Down Expand Up @@ -2296,7 +2296,7 @@ public Construct exec(Target t, Environment environment, Construct... args) thro
if(ret == null) {
ret = CBoolean.FALSE;
}
boolean bret = Static.getBoolean(ret);
boolean bret = Static.getBoolean(ret, t);
if(bret) {
newArray.set(key, value, t);
}
Expand All @@ -2315,7 +2315,7 @@ public Construct exec(Target t, Environment environment, Construct... args) thro
if(ret == null) {
ret = CBoolean.FALSE;
}
boolean bret = Static.getBoolean(ret);
boolean bret = Static.getBoolean(ret, t);
if(bret) {
newArray.push(value, t);
}
Expand Down Expand Up @@ -2798,7 +2798,7 @@ public Construct exec(Target t, Environment environment, Construct... args) thro
closure.execute(array.get(c, t));
} catch(FunctionReturnException ex) {
hasReturn = true;
boolean ret = Static.getBoolean(ex.getReturn());
boolean ret = Static.getBoolean(ex.getReturn(), t);
if(ret == false) {
return CBoolean.FALSE;
}
Expand Down Expand Up @@ -2879,7 +2879,7 @@ public Construct exec(Target t, Environment environment, Construct... args) thro
closure.execute(array.get(c, t));
} catch(FunctionReturnException ex) {
hasReturn = true;
boolean ret = Static.getBoolean(ex.getReturn());
boolean ret = Static.getBoolean(ex.getReturn(), t);
if(ret == true) {
return CBoolean.TRUE;
}
Expand Down
Loading

0 comments on commit 7dd4957

Please sign in to comment.