From cb6f24783c07e100dbb24b4c1b905078e4cb3841 Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Mon, 14 Apr 2025 01:31:54 +0200 Subject: [PATCH 01/11] Redefine ICountable.matches, getDeprecationAnnotation is not part of the "engine" character of that interface --- .../unciv/models/ruleset/unique/Countables.kt | 91 +++++++++++++------ .../ruleset/validation/UniqueValidator.kt | 5 +- tests/src/com/unciv/uniques/CountableTests.kt | 17 +--- 3 files changed, 70 insertions(+), 43 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index cba68d97f9a3c..018578a05398f 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -11,20 +11,40 @@ import org.jetbrains.annotations.VisibleForTesting * Prototype for each new [Countables] instance, core functionality, to ensure a baseline. * * Notes: - * - Each instance ***must*** implement _either_ overload of [matches] and indicate which one via [matchesWithRuleset]. * - [matches] is used to look up which instance implements a given string, **without** validating its placeholders. + * It can be called with or without a ruleset. The ruleset is **only** to be used if there is no selective pattern + * to detect when a specific countable is "responsible" for a certain input, and for these, when `matches` is called + * without a ruleset, they must return `MatchResult.No` (Example below: TileResource). * - [getErrorSeverity] is responsible for validating placeholders, _and can assume [matches] was successful_. * - Override [getKnownValuesForAutocomplete] only if a sensible number of suggestions is obvious. */ interface ICountable { - fun matches(parameterText: String): Boolean = false - val matchesWithRuleset: Boolean - get() = false - fun matches(parameterText: String, ruleset: Ruleset): Boolean = false + /** Supports `MatchResult(true)`to get [Yes], MatchResult(false)`to get [No], or MatchResult(null)`to get [Maybe] */ + enum class MatchResult { + No { + override fun isOK(strict: Boolean) = false + }, + Maybe { + override fun isOK(strict: Boolean) = !strict + }, + Yes { + override fun isOK(strict: Boolean) = true + } + ; + abstract fun isOK(strict: Boolean): Boolean + companion object { + operator fun invoke(bool: Boolean?) = when(bool) { + true -> Yes + false -> No + else -> Maybe + } + } + } + + fun matches(parameterText: String, ruleset: Ruleset? = null): MatchResult = MatchResult.No fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? fun getKnownValuesForAutocomplete(ruleset: Ruleset): Set = emptySet() fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? - fun getDeprecationAnnotation(): Deprecated? } /** @@ -54,7 +74,7 @@ enum class Countables( ) : ICountable { Integer { override val documentationHeader = "Integer constant - any positive or negative integer number" - override fun matches(parameterText: String) = parameterText.toIntOrNull() != null + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(parameterText.toIntOrNull() != null) override fun eval(parameterText: String, stateForConditionals: StateForConditionals) = parameterText.toIntOrNull() }, @@ -80,7 +100,7 @@ enum class Countables( Stats { override val documentationHeader = "Stat name (${Stat.names().niceJoin()})" override val documentationStrings = listOf("gets the stat *reserve*, not the amount per turn (can be city stats or civilization stats, depending on where the unique is used)") - override fun matches(parameterText: String) = Stat.isStat(parameterText) + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(Stat.isStat(parameterText)) override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? { val relevantStat = Stat.safeValueOf(parameterText) ?: return null // This one isn't covered by City.getStatReserve or Civilization.getStatReserve but should be available here @@ -164,8 +184,7 @@ enum class Countables( "For example: If a unique is placed on a building, then the retrieved resources will be of the city. If placed on a policy, they will be of the civilization.", "This can make a difference for e.g. local resources, which are counted per city." ) - override val matchesWithRuleset = true - override fun matches(parameterText: String, ruleset: Ruleset) = parameterText in ruleset.tileResources + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(ruleset?.tileResources?.containsKey(parameterText)) override fun eval(parameterText: String, stateForConditionals: StateForConditionals) = stateForConditionals.getResourceAmount(parameterText) override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = ruleset.tileResources.keys @@ -179,16 +198,20 @@ enum class Countables( return civilizations.count { it.isAlive() && it.isCityState } } } - ; + ; ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + //region ' class-wide elements and ICountable' val placeholderText = text.getPlaceholderText() @VisibleForTesting open val noPlaceholders = !text.contains('[') // Leave these in place only for the really simple cases - override fun matches(parameterText: String) = if (noPlaceholders) parameterText == text + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult( + if (noPlaceholders) parameterText == text else parameterText.equalsPlaceholderText(placeholderText) + ) + override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = setOf(text) open val documentationHeader get() = @@ -197,23 +220,24 @@ enum class Countables( /** Leave this only for Countables without any parameters - they can rely on [matches] having validated enough */ override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = null - override fun getDeprecationAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) - + /** Helper for Countables with exactly one placeholder that is a UniqueParameterType */ protected fun UniqueParameterType.getTranslatedErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { + // This calls UniqueParameterType's getErrorSeverity: val severity = getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) + // Map PossibleFilteringUnique to RulesetSpecific otherwise those mistakes would be hidden later on in RulesetValidator return when { severity != UniqueType.UniqueParameterErrorSeverity.PossibleFilteringUnique -> severity - matchesWithRuleset -> UniqueType.UniqueParameterErrorSeverity.RulesetSpecific - else -> UniqueType.UniqueParameterErrorSeverity.RulesetInvariant + else -> UniqueType.UniqueParameterErrorSeverity.RulesetSpecific } } + fun getDeprecationAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) + //endregion + companion object { - fun getMatching(parameterText: String, ruleset: Ruleset?) = Countables.entries + private fun getMatching(parameterText: String, ruleset: Ruleset?) = Countables.entries .filter { - if (it.matchesWithRuleset) - ruleset != null && it.matches(parameterText, ruleset!!) - else it.matches(parameterText) + it.matches(parameterText, ruleset).isOK(strict = true) } fun getCountableAmount(parameterText: String, stateForConditionals: StateForConditionals): Int? { @@ -237,13 +261,28 @@ enum class Countables( } fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { - var result = UniqueType.UniqueParameterErrorSeverity.RulesetInvariant - for (countable in Countables.getMatching(parameterText, ruleset)) { - // If any Countable is happy, we're happy - result = countable.getErrorSeverity(parameterText, ruleset) ?: return null + var result: UniqueType.UniqueParameterErrorSeverity? = null + for (countable in Countables.entries) { + val thisResult = when (countable.matches(parameterText, ruleset)) { + ICountable.MatchResult.No -> continue + ICountable.MatchResult.Yes -> + countable.getErrorSeverity(parameterText, ruleset) + // If any Countable is happy, we're happy: Should be the only path to return `null`, meaning perfectly OK + ?: return null + else -> UniqueType.UniqueParameterErrorSeverity.PossibleFilteringUnique + } + if (result == null || thisResult > result) result = thisResult } - // return last result or default for simplicity - could do a max() instead - return result + // return worst result - or if the loop found nothing, max severity + return result ?: UniqueType.UniqueParameterErrorSeverity.RulesetInvariant } + + /** Get deprecated [Countables] with their [Deprecated] object matching a [parameterText], for `UniqueValidator` */ + fun getDeprecatedCountablesMatching(parameterText: String): List> = + Countables.entries.filter { + it.matches(parameterText, null).isOK(strict = false) + }.mapNotNull { countable -> + countable.getDeprecationAnnotation()?.let { countable to it } + } } } diff --git a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt index b2bb85cd674f7..56742615c9e67 100644 --- a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt +++ b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt @@ -252,9 +252,8 @@ class UniqueValidator(val ruleset: Ruleset) { unique.type.parameterTypeMap.withIndex() .filter { UniqueParameterType.Countable in it.value } .map { unique.params[it.index] } - .flatMap { Countables.getMatching(it, ruleset) } - for (countable in countables) { - val deprecation = countable.getDeprecationAnnotation() ?: continue + .flatMap { Countables.getDeprecatedCountablesMatching(it) } + for ((countable, deprecation) in countables) { // This is less flexible than unique.getReplacementText(ruleset) val replaceExpression = deprecation.replaceWith.expression val text = "Countable `${countable.name}` is deprecated ${deprecation.message}" + diff --git a/tests/src/com/unciv/uniques/CountableTests.kt b/tests/src/com/unciv/uniques/CountableTests.kt index 6ae7b8551cb06..5a39279758598 100644 --- a/tests/src/com/unciv/uniques/CountableTests.kt +++ b/tests/src/com/unciv/uniques/CountableTests.kt @@ -44,20 +44,9 @@ class CountableTests { for (instance in Countables::class.java.enumConstants) { val instanceClazz = instance::class.java - val matchesRulesetOverridden = instanceClazz.hasOverrideFor("matches", String::class.java, Ruleset::class.java) - val matchesPlainOverridden = instanceClazz.hasOverrideFor("matches", String::class.java) - if (instance.matchesWithRuleset && !matchesRulesetOverridden) { - println("`$instance` is marked as working _with_ a `Ruleset` but fails to override `matches(String,Ruleset)`,") - fails++ - } else if (instance.matchesWithRuleset && matchesPlainOverridden) { - println("`$instance` is marked as working _with_ a `Ruleset` but overrides `matches(String)` which is worthless.") - fails++ - } else if (!instance.matchesWithRuleset && matchesRulesetOverridden) { - println("`$instance` is marked as working _without_ a `Ruleset` but overrides `matches(String,Ruleset)` which is worthless.") - fails++ - } - if (instance.text.isEmpty() && !matchesPlainOverridden && !matchesRulesetOverridden) { - println("`$instance` has no `text` but fails to override either `matches` overload.") + val matchesOverridden = instanceClazz.hasOverrideFor("matches", String::class.java, Ruleset::class.java) + if (instance.text.isEmpty() && !matchesOverridden) { + println("`$instance` has no `text` but fails to override `matches`.") fails++ } From c9f27b895cba5ed8dda9a63012778dc57a401c6b Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Mon, 14 Apr 2025 02:03:41 +0200 Subject: [PATCH 02/11] Add a way to mark not-yet-finished Countables --- .../unciv/models/ruleset/unique/Countables.kt | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index 018578a05398f..529a233bbeaad 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -5,7 +5,11 @@ import com.unciv.models.stats.Stat import com.unciv.models.translations.equalsPlaceholderText import com.unciv.models.translations.getPlaceholderParameters import com.unciv.models.translations.getPlaceholderText +import com.unciv.utils.Log import org.jetbrains.annotations.VisibleForTesting +import java.time.LocalDate +import java.time.ZoneOffset +import java.time.format.DateTimeFormatter /** * Prototype for each new [Countables] instance, core functionality, to ensure a baseline. @@ -230,14 +234,41 @@ enum class Countables( else -> UniqueType.UniqueParameterErrorSeverity.RulesetSpecific } } + //endregion + + //region ' Deprecation and InDevelopment' + fun getDeprecationAnnotation(): Deprecated? = getDeprecatedAnnotation() ?: getDeprecationFromInDevelopment() + + /** + * This annotation marks a [Countables] instance as active for debug builds only. + * - If you add both @Deprecated and @InDevelopment to the same instance, the validator will only see the @Deprecated one, + * while @InDevelopment still controls whether it si active (evaluated at all). + * @param developer - the responsible dev as `@` plus github account + * @param eta - An expiration date in the form 'yyyy-mm-dd', UTC, after which the instance is treated as deprecated **and** inactive + */ + @Retention(AnnotationRetention.RUNTIME) + @Target(AnnotationTarget.FIELD) + annotation class InDevelopment(val developer: String, val eta: String) - fun getDeprecationAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) + private fun getDeprecatedAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) + private fun getInDevelopmentAnnotation(): InDevelopment? = declaringJavaClass.getField(name).getAnnotation(InDevelopment::class.java) + private fun InDevelopment.isExpired(): Boolean { + val etaDate = LocalDate.parse(eta, DateTimeFormatter.ofPattern("yyyy-MM-dd")) + return etaDate.isBefore(LocalDate.now(ZoneOffset.UTC)) + } + private fun isInactive(): Boolean { + val inDev = getInDevelopmentAnnotation() ?: return false + if (inDev.isExpired()) return true + return Log.backend.isRelease() + } + private fun getDeprecationFromInDevelopment(): Deprecated? = + if (getInDevelopmentAnnotation()?.isExpired() == true) Deprecated("InDevelopment ETA has expired") else null //endregion companion object { private fun getMatching(parameterText: String, ruleset: Ruleset?) = Countables.entries .filter { - it.matches(parameterText, ruleset).isOK(strict = true) + !it.isInactive() && it.matches(parameterText, ruleset).isOK(strict = true) } fun getCountableAmount(parameterText: String, stateForConditionals: StateForConditionals): Int? { @@ -263,6 +294,7 @@ enum class Countables( fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { var result: UniqueType.UniqueParameterErrorSeverity? = null for (countable in Countables.entries) { + if (countable.isInactive()) continue val thisResult = when (countable.matches(parameterText, ruleset)) { ICountable.MatchResult.No -> continue ICountable.MatchResult.Yes -> @@ -280,7 +312,7 @@ enum class Countables( /** Get deprecated [Countables] with their [Deprecated] object matching a [parameterText], for `UniqueValidator` */ fun getDeprecatedCountablesMatching(parameterText: String): List> = Countables.entries.filter { - it.matches(parameterText, null).isOK(strict = false) + !it.isInactive() && it.matches(parameterText, null).isOK(strict = false) }.mapNotNull { countable -> countable.getDeprecationAnnotation()?.let { countable to it } } From 6d95ea3f195240ed652bb171600101d11e3cb343 Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Mon, 14 Apr 2025 02:04:59 +0200 Subject: [PATCH 03/11] Add an empty framework for @AutumnPizazz's Expression engine --- .../unciv/models/ruleset/unique/Countables.kt | 23 +++++++++++++++++++ .../models/ruleset/unique/Expressions.kt | 19 +++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 core/src/com/unciv/models/ruleset/unique/Expressions.kt diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index 529a233bbeaad..3b927884546c0 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -201,6 +201,29 @@ enum class Countables( val civilizations = stateForConditionals.gameInfo?.civilizations ?: return null return civilizations.count { it.isAlive() && it.isCityState } } + }, + + @InDevelopment("@AutumnPizazz", eta = "2025-06-30") + Expression { + override val noPlaceholders = false + + private val engine = Expressions() + + override fun matches(parameterText: String, ruleset: Ruleset?) = + engine.matches(parameterText, ruleset) + override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? = + engine.eval(parameterText, stateForConditionals) + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = + engine.getErrorSeverity(parameterText, ruleset) + + override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = emptySet() + + override val documentationHeader = "Evaluate expressions!" + override val documentationStrings = listOf( + "Expressions support `+`, `-`, `*`, `/`, `%`, `^` and `log` as binary operators.", + "Operands can be floating point constants or other countables in square brackets", + "..." + ) } ; ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/core/src/com/unciv/models/ruleset/unique/Expressions.kt b/core/src/com/unciv/models/ruleset/unique/Expressions.kt new file mode 100644 index 0000000000000..8208d22cb4db4 --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/Expressions.kt @@ -0,0 +1,19 @@ +package com.unciv.models.ruleset.unique + +import com.unciv.models.ruleset.Ruleset + +class Expressions : ICountable { + //TODO these are all placeholders! + + override fun matches(parameterText: String, ruleset: Ruleset?): ICountable.MatchResult { + return ICountable.MatchResult.No + } + + override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? { + return null + } + + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { + return UniqueType.UniqueParameterErrorSeverity.RulesetInvariant + } +} From eee7dd0a14c53b68bed4449784b5c5a590c976ca Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Mon, 14 Apr 2025 02:19:43 +0200 Subject: [PATCH 04/11] Make "which does not fit parameter type" constant for pattern match robustness --- core/src/com/unciv/models/ruleset/RulesetCache.kt | 9 +++++++-- .../unciv/models/ruleset/validation/UniqueValidator.kt | 7 ++++--- tests/src/com/unciv/uniques/CountableTests.kt | 3 ++- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/RulesetCache.kt b/core/src/com/unciv/models/ruleset/RulesetCache.kt index 9a2e7fa57dca3..ace287f20de26 100644 --- a/core/src/com/unciv/models/ruleset/RulesetCache.kt +++ b/core/src/com/unciv/models/ruleset/RulesetCache.kt @@ -10,6 +10,7 @@ import com.unciv.models.metadata.GameParameters import com.unciv.models.ruleset.validation.RulesetError import com.unciv.models.ruleset.validation.RulesetErrorList import com.unciv.models.ruleset.validation.RulesetErrorSeverity +import com.unciv.models.ruleset.validation.UniqueValidator import com.unciv.models.ruleset.validation.getRelativeTextDistance import com.unciv.utils.Log import com.unciv.utils.debug @@ -59,8 +60,12 @@ object RulesetCache : HashMap() { // For extension mods which use references to base ruleset objects, the parameter type // errors are irrelevant - the checker ran without a base ruleset val logFilter: (RulesetError) -> Boolean = - if (modRuleset.modOptions.isBaseRuleset) { { it.errorSeverityToReport > RulesetErrorSeverity.WarningOptionsOnly } } - else { { it.errorSeverityToReport > RulesetErrorSeverity.WarningOptionsOnly && !it.text.contains("does not fit parameter type") } } + if (modRuleset.modOptions.isBaseRuleset) { + { it.errorSeverityToReport > RulesetErrorSeverity.WarningOptionsOnly } + } else { + { it.errorSeverityToReport > RulesetErrorSeverity.WarningOptionsOnly && + !it.text.contains(UniqueValidator.whichDoesNotFitParameterType) } + } if (modLinksErrors.any(logFilter)) { debug( "checkModLinks errors: %s", diff --git a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt index 56742615c9e67..7a9dfb31b8235 100644 --- a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt +++ b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt @@ -81,8 +81,7 @@ class UniqueValidator(val ruleset: Ruleset) { continue rulesetErrors.add( - "$prefix contains parameter ${complianceError.parameterName}," + - " which does not fit parameter type" + + "$prefix contains parameter \"${complianceError.parameterName}\", $whichDoesNotFitParameterType" + " ${complianceError.acceptableParameterTypes.joinToString(" or ") { it.parameterName }} !", complianceError.errorSeverity.getRulesetErrorSeverity(), uniqueContainer, unique ) @@ -218,7 +217,7 @@ class UniqueValidator(val ruleset: Ruleset) { rulesetErrors.add( "$prefix contains modifier \"${conditional.text}\"." + - " This contains the parameter \"${complianceError.parameterName}\" which does not fit parameter type" + + " This contains the parameter \"${complianceError.parameterName}\" $whichDoesNotFitParameterType" + " ${complianceError.acceptableParameterTypes.joinToString(" or ") { it.parameterName }} !", complianceError.errorSeverity.getRulesetErrorSeverity(), uniqueContainer, unique ) @@ -368,6 +367,8 @@ class UniqueValidator(val ruleset: Ruleset) { } companion object { + const val whichDoesNotFitParameterType = "which does not fit parameter type" + internal fun getUniqueContainerPrefix(uniqueContainer: IHasUniques?) = (if (uniqueContainer is IRulesetObject) "${uniqueContainer.originRuleset}: " else "") + (if (uniqueContainer == null) "The" else "(${uniqueContainer.getUniqueTarget().name}) ${uniqueContainer.name}'s") + diff --git a/tests/src/com/unciv/uniques/CountableTests.kt b/tests/src/com/unciv/uniques/CountableTests.kt index 5a39279758598..b516c0a84892c 100644 --- a/tests/src/com/unciv/uniques/CountableTests.kt +++ b/tests/src/com/unciv/uniques/CountableTests.kt @@ -9,6 +9,7 @@ import com.unciv.models.ruleset.unique.Unique import com.unciv.models.ruleset.unique.UniqueParameterType import com.unciv.models.ruleset.unique.UniqueTriggerActivation import com.unciv.models.ruleset.validation.RulesetValidator +import com.unciv.models.ruleset.validation.UniqueValidator import com.unciv.models.stats.Stat import com.unciv.models.translations.getPlaceholderParameters import com.unciv.testing.GdxTestRunner @@ -209,7 +210,7 @@ class CountableTests { "[+1 Food] " to 3, ) val totalNotACountableExpected = testData.sumOf { it.second } - val notACountableRegex = Regex(""".*parameter "(.*)" which does not fit parameter type countable.*""") + val notACountableRegex = Regex(""".*parameter "(.*)" ${UniqueValidator.whichDoesNotFitParameterType} countable.*""") val ruleset = setupModdedGame( *testData.map { it.first }.toTypedArray(), From adfa8bf762b145dbb1caede299c6de39fa77bb19 Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Tue, 15 Apr 2025 23:07:41 +0200 Subject: [PATCH 05/11] New expression evaluator engine --- .../unciv/models/ruleset/unique/Countables.kt | 11 ++ .../models/ruleset/unique/Expressions.kt | 19 --- .../ruleset/unique/expressions/Expressions.kt | 39 +++++ .../models/ruleset/unique/expressions/Node.kt | 35 +++++ .../ruleset/unique/expressions/Operator.kt | 108 ++++++++++++++ .../ruleset/unique/expressions/Parser.kt | 140 ++++++++++++++++++ .../ruleset/unique/expressions/Tokenizer.kt | 120 +++++++++++++++ .../src/com/unciv/uniques/ExpressionTests.kt | 122 +++++++++++++++ 8 files changed, 575 insertions(+), 19 deletions(-) delete mode 100644 core/src/com/unciv/models/ruleset/unique/Expressions.kt create mode 100644 core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt create mode 100644 core/src/com/unciv/models/ruleset/unique/expressions/Node.kt create mode 100644 core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt create mode 100644 core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt create mode 100644 core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt create mode 100644 tests/src/com/unciv/uniques/ExpressionTests.kt diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index 3b927884546c0..50744d81e0698 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -1,6 +1,7 @@ package com.unciv.models.ruleset.unique import com.unciv.models.ruleset.Ruleset +import com.unciv.models.ruleset.unique.expressions.Expressions import com.unciv.models.stats.Stat import com.unciv.models.translations.equalsPlaceholderText import com.unciv.models.translations.getPlaceholderParameters @@ -294,6 +295,16 @@ enum class Countables( !it.isInactive() && it.matches(parameterText, ruleset).isOK(strict = true) } + fun getBestMatching(parameterText: String, ruleset: Ruleset?): Pair = + Countables.entries + .filter { + !it.isInactive() && it.matches(parameterText, ruleset).isOK(strict = true) + }.mapNotNull { + val result = it.matches(parameterText, ruleset) + if (result.isOK(strict = false)) it to result else null + }.minByOrNull { it.second } + ?: Pair(null, ICountable.MatchResult.No) + fun getCountableAmount(parameterText: String, stateForConditionals: StateForConditionals): Int? { val ruleset = stateForConditionals.gameInfo?.ruleset for (countable in Countables.getMatching(parameterText, ruleset)) { diff --git a/core/src/com/unciv/models/ruleset/unique/Expressions.kt b/core/src/com/unciv/models/ruleset/unique/Expressions.kt deleted file mode 100644 index 8208d22cb4db4..0000000000000 --- a/core/src/com/unciv/models/ruleset/unique/Expressions.kt +++ /dev/null @@ -1,19 +0,0 @@ -package com.unciv.models.ruleset.unique - -import com.unciv.models.ruleset.Ruleset - -class Expressions : ICountable { - //TODO these are all placeholders! - - override fun matches(parameterText: String, ruleset: Ruleset?): ICountable.MatchResult { - return ICountable.MatchResult.No - } - - override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? { - return null - } - - override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { - return UniqueType.UniqueParameterErrorSeverity.RulesetInvariant - } -} diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt new file mode 100644 index 0000000000000..6a3f919ed98ba --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt @@ -0,0 +1,39 @@ +package com.unciv.models.ruleset.unique.expressions + +import com.unciv.models.ruleset.Ruleset +import com.unciv.models.ruleset.unique.ICountable +import com.unciv.models.ruleset.unique.StateForConditionals +import com.unciv.models.ruleset.unique.UniqueType +import kotlin.math.roundToInt + +class Expressions : ICountable { + override fun matches(parameterText: String, ruleset: Ruleset?) = + parse(parameterText, ruleset).severity + + override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? { + val node = parse(parameterText, null).node ?: return null + return node.eval(stateForConditionals).roundToInt() + } + + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset) = + when(parse(parameterText, ruleset).severity) { + ICountable.MatchResult.No -> UniqueType.UniqueParameterErrorSeverity.RulesetInvariant + ICountable.MatchResult.Maybe -> UniqueType.UniqueParameterErrorSeverity.PossibleFilteringUnique + ICountable.MatchResult.Yes -> null + } + + private data class ParseResult(val severity: ICountable.MatchResult, val node: Node?) + + companion object { + private val cache: MutableMap = mutableMapOf() + + private fun parse(parameterText: String, ruleset: Ruleset?): ParseResult = cache.getOrPut(parameterText) { + try { + val node = Parser.parse(parameterText, ruleset) + ParseResult(ICountable.MatchResult.Yes, node) + } catch (ex: Parser.ParsingError) { + ParseResult(ex.severity, null) + } + } + } +} diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Node.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Node.kt new file mode 100644 index 0000000000000..5bfc9a6483ba8 --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Node.kt @@ -0,0 +1,35 @@ +package com.unciv.models.ruleset.unique.expressions + +import com.unciv.models.ruleset.unique.Countables +import com.unciv.models.ruleset.unique.StateForConditionals + +internal sealed interface Node { + fun eval(context: StateForConditionals): Double + + // All elements below are not members, they're nested for namespace notation and common visibility + // All toString() are for debugging only + + interface Constant : Node, Tokenizer.Token { + val value: Double + override fun eval(context: StateForConditionals) = value + } + + class NumericConstant(override val value: Double) : Constant { + override fun toString() = value.toString() + } + + class UnaryOperation(private val operator: Operator.Unary, private val operand: Node): Node { + override fun eval(context: StateForConditionals): Double = operator.implementation(operand.eval(context)) + override fun toString() = "($operator $operand)" + } + + class BinaryOperation(private val operator: Operator.Binary, private val left: Node, private val right: Node): Node { + override fun eval(context: StateForConditionals): Double = operator.implementation(left.eval(context), right.eval(context)) + override fun toString() = "($left $operator $right)" + } + + class Countable(private val countable: Countables, private val parameterText: String): Node, Tokenizer.Token { + override fun eval(context: StateForConditionals): Double = countable.eval(parameterText, context)?.toDouble() ?: 0.0 + override fun toString() = "[Countables.${countable.name}: $parameterText]" + } +} diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt new file mode 100644 index 0000000000000..f927d83d524e2 --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt @@ -0,0 +1,108 @@ +package com.unciv.models.ruleset.unique.expressions + +import kotlin.math.abs +import kotlin.math.atan +import kotlin.math.cbrt +import kotlin.math.cos +import kotlin.math.exp +import kotlin.math.ln +import kotlin.math.log10 +import kotlin.math.pow +import kotlin.math.sin +import kotlin.math.sqrt +import kotlin.math.tan + +internal sealed interface Operator : Tokenizer.Token { + val symbol: String + + // All elements below are not members, they're nested for namespace notation and common visibility + // All toString() are for use in exception messages only + + interface Unary : Operator { + val implementation: (Double) -> Double + } + + interface Binary : Operator { + val precedence: Int + val isLeftAssociative: Boolean + val implementation: (Double, Double) -> Double + } + + interface UnaryOrBinary : Operator { + val unary: Unary + val binary: Binary + } + + enum class UnaryOperators( + override val symbol: String, + override val implementation: (Double) -> Double + ) : Unary { + Identity("+", { operand -> operand }), + Negation("-", { operand -> -operand }), + Sqrt("√", ::sqrt), + Abs("abs", ::abs), + Sqrt2("sqrt", ::sqrt), + Cbrt("cbtr", ::cbrt), + Exp("exp", ::exp), + Log("ln", ::ln), + Log10("log10", ::log10), + Sin("sin", ::sin), + Cos("cos", ::cos), + Tan("tan", ::tan), + Atan("atan", ::atan), + ; + override fun toString() = symbol + } + + enum class BinaryOperators( + override val symbol: String, + override val precedence: Int, + override val isLeftAssociative: Boolean, + override val implementation: (Double, Double) -> Double + ) : Binary { + Addition("+", 2, true, { left, right -> left + right }), + Subtraction("-", 2, true, { left, right -> left - right }), + Multiplication("*", 3, true, { left, right -> left * right }), + Division("/", 3, true, { left, right -> left / right }), + Remainder("%", 3, true, { left, right -> ((left % right) + right) % right }), // true modulo, always non-negative + Exponent("^", 4, false, { left, right -> left.pow(right) }), + ; + override fun toString() = symbol + } + + enum class UnaryOrBinaryOperators( + override val symbol: String, + override val unary: Unary, + override val binary: Binary + ) : UnaryOrBinary { + Plus("+", UnaryOperators.Identity, BinaryOperators.Addition), + Minus("-", UnaryOperators.Negation, BinaryOperators.Subtraction), + ; + override fun toString() = symbol + } + + enum class NamedConstants(override val symbol: String, override val value: Double) : Node.Constant, Operator { + Pi("pi", kotlin.math.PI), + Pi2("π", kotlin.math.PI), + Euler("e", kotlin.math.E), + ; + override fun toString() = symbol + } + + enum class Parentheses(override val symbol: String) : Operator { + Opening("("), Closing(")") + ; + override fun toString() = symbol + } + + companion object { + private fun allEntries(): Sequence = + UnaryOperators.entries.asSequence() + + BinaryOperators.entries + + UnaryOrBinaryOperators.entries + // Will overwrite the previous entries in the map + NamedConstants.entries + + Parentheses.entries + private val cache = allEntries().associateBy { it.symbol } + fun of(symbol: String): Operator? = cache[symbol] + } +} diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt new file mode 100644 index 0000000000000..aac57fb737617 --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt @@ -0,0 +1,140 @@ +package com.unciv.models.ruleset.unique.expressions + +import com.unciv.models.ruleset.Ruleset +import com.unciv.models.ruleset.unique.Countables +import com.unciv.models.ruleset.unique.ICountable +import com.unciv.models.ruleset.unique.StateForConditionals +import com.unciv.models.ruleset.unique.expressions.Operator.Parentheses +import com.unciv.models.ruleset.unique.expressions.Tokenizer.Token +import com.unciv.models.ruleset.unique.expressions.Tokenizer.toToken +import com.unciv.models.ruleset.unique.expressions.Tokenizer.tokenize +import org.jetbrains.annotations.VisibleForTesting + +/** + * Parse and evaluate simple expressions + * - [eval] Does a one-off AST conversion and evaluation in one go + * - [parse] Builds the AST without evaluating + * - Supports [Countables] as terms, enclosed in square brackets (they're optional when the countable is a single identifier!). + * + * Very freely inspired by [Keval](https://github.com/notKamui/Keval). + * + * ##### Current Limitations: + * - Non-Alphanumeric tokens are always one character (ie needs work to support `<=` and similar operators). + * - Numeric constants do not support scientific notation. + * - Alphanumeric identifiers (can be matched with simple countables or function names) can _only_ contain letters and digits as defined by defined by unicode properties, and '_'. + * - Functions with arity > 1 aren't supported. No parameter lists with comma - in fact, functions are just implemented as infix operators. + * - Only prefix Unary operators, e.g. no standard factorial notation. + */ +object Parser { + /** + * Parse and evaluate an expression. If it needs to support countables, [ruleset] and [context] should be supplied. + */ + fun eval(text: String, ruleset: Ruleset? = null, context: StateForConditionals = StateForConditionals.EmptyState): Double = + parse(text, ruleset).eval(context) + + internal fun parse(text: String, ruleset: Ruleset?): Node { + val tokens = text.tokenize().map { it.toToken(ruleset) } + val engine = StateEngine(tokens) + return engine.buildAST() + } + + @VisibleForTesting + fun getASTDebugDescription(text: String, ruleset: Ruleset?) = + parse(text, ruleset).toString() + + //region Exceptions + /** Parent of all exceptions [parse] can throw. + * If the exception caught is not [SyntaxError], then an Expression Countable should say NO "that can't possibly an expression". */ + open class ParsingError(override val message: String, open val severity: ICountable.MatchResult = ICountable.MatchResult.No) : Exception() + /** Less severe than [ParsingError]. + * It allows an Expression Countable to say "Maybe", meaning the string might be of type Expression, but malformed. */ + open class SyntaxError(message: String) : ParsingError(message, ICountable.MatchResult.Maybe) + class UnmatchedBraces(pos: Int) : ParsingError("Unmatched square braces at $pos") + class EmptyBraces : ParsingError("Empty square braces") + class UnmatchedParentheses(name: String) : SyntaxError("Unmatched $name parenthesis") + internal class UnexpectedToken(expected: Token, found: Token) : ParsingError("Unexpected token: $found instead of $expected") + class MissingOperand : SyntaxError("Missing operand") + class InvalidConstant(text: String) : SyntaxError("Invalid constant: $text") + class MalformedCountable(countable: Countables, text: String) : SyntaxError("Token \"$text\" seems to be a Countable(${countable.name}), but is malformed") + class UnrecognizedToken : ParsingError("Unrecognized token") + class InternalError : ParsingError("Internal error") + //endregion + + /** Marker for beginning of the expression */ + private data object StartToken : Token { + override fun toString() = "start of expression" + } + /** Marker for end of the expression */ + private data object EndToken : Token { + override fun toString() = "end of expression" + } + + private class StateEngine(input: Sequence) { + private var currentToken: Token = StartToken + private val iterator = input.iterator() + private var openParenthesesCount = 0 + + private fun expect(expected: Token) { + if (currentToken == expected) return + if (expected == Parentheses.Closing && currentToken == EndToken) + throw UnmatchedParentheses(Parentheses.Opening.name.lowercase()) + if (expected == EndToken && currentToken == Parentheses.Closing) + throw UnmatchedParentheses(Parentheses.Closing.name.lowercase()) + throw UnexpectedToken(expected, currentToken) + } + + private fun next() { + if (currentToken == Parentheses.Opening) { + openParenthesesCount++ + } else if (currentToken == Parentheses.Closing) { + if (openParenthesesCount == 0) + throw UnmatchedParentheses(Parentheses.Closing.name.lowercase()) + openParenthesesCount-- + } + currentToken = if (iterator.hasNext()) iterator.next() else EndToken + } + + private fun handleUnary(): Node { + val operator = currentToken.fetchUnaryOperator() + next() + return Node.UnaryOperation(operator, fetchOperand()) + } + + private fun expression(minPrecedence: Int = 0): Node { + var result = fetchOperand() + while (currentToken.canBeBinary()) { + val operator = currentToken.fetchBinaryOperator() + if (operator.precedence < minPrecedence) break + next() + val newPrecedence = if (operator.isLeftAssociative) operator.precedence + 1 else operator.precedence + result = Node.BinaryOperation(operator, result, expression(newPrecedence)) + } + return result + } + + private fun fetchOperand(): Node { + if (currentToken == StartToken) next() + if (currentToken.canBeUnary()) { + return handleUnary() + } else if (currentToken == Parentheses.Opening) { + next() + val node = expression() + expect(Parentheses.Closing) + next() + return node + } else if (currentToken is Node.Constant || currentToken is Node.Countable) { + val node = currentToken as Node + next() + return node + } else { + throw MissingOperand() + } + } + + fun buildAST(): Node { + val node = expression() + expect(EndToken) + return node + } + } +} diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt new file mode 100644 index 0000000000000..fea3bcf4e57b0 --- /dev/null +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt @@ -0,0 +1,120 @@ +package com.unciv.models.ruleset.unique.expressions + +import com.unciv.models.ruleset.Ruleset +import com.unciv.models.ruleset.unique.Countables +import com.unciv.models.ruleset.unique.ICountable +import com.unciv.models.ruleset.unique.expressions.Operator.Parentheses +import com.unciv.models.ruleset.unique.expressions.Parser.EmptyBraces +import com.unciv.models.ruleset.unique.expressions.Parser.InternalError +import com.unciv.models.ruleset.unique.expressions.Parser.InvalidConstant +import com.unciv.models.ruleset.unique.expressions.Parser.MalformedCountable +import com.unciv.models.ruleset.unique.expressions.Parser.UnmatchedBraces +import com.unciv.models.ruleset.unique.expressions.Parser.UnrecognizedToken + +internal object Tokenizer { + /** + * Possible types: + * - [Parentheses] (defined in [Operator] - for convenience, they conform to the minimal interface) + * - [Operator] + * - [Node.Constant] + * - [Node.Countable] + */ + internal sealed interface Token { + fun canBeUnary() = this is Operator.Unary || this is Operator.UnaryOrBinary + fun canBeBinary() = this is Operator.Binary || this is Operator.UnaryOrBinary + // Note: not naming this `getUnaryOperator` because of kotlin's habit to interpret that as property accessor. Messes up debugging a bit. + fun fetchUnaryOperator() = when(this) { + is Operator.Unary -> this + is Operator.UnaryOrBinary -> unary + else -> throw InternalError() + } + fun fetchBinaryOperator() = when(this) { + is Operator.Binary -> this + is Operator.UnaryOrBinary -> binary + else -> throw InternalError() + } + } + + // Define our own "Char is part of literal constant" and "Char is part of identifier" functions - decouple from Java CharacterData + private fun Char.isNumberLiteral() = this == '.' || this in '0'..'9' // NOT using library isDigit() here - potentially non-latin + private fun Char.isIdentifierStart() = isLetter() // Allow potentially non-latin script //TODO questionable + private fun Char.isIdentifierContinuation() = this == '_' || isLetterOrDigit() + + fun String.toToken(ruleset: Ruleset?): Token { + assert(isNotBlank()) + if (first().isDigit() || first() == '.') + return Node.NumericConstant(toDouble()) + val operator = Operator.of(this) + if (operator != null) return operator + // Countable tokens must come here still wrapped in braces to avoid infinite recursion + if(!startsWith('[') || !endsWith(']')) + throw UnrecognizedToken() + val countableText = substring(1, length - 1) + val (countable, severity) = Countables.getBestMatching(countableText, ruleset) + if (severity == ICountable.MatchResult.Yes) + return Node.Countable(countable!!, countableText) + if (severity == ICountable.MatchResult.Maybe) + throw MalformedCountable(countable!!, countableText) + throw UnrecognizedToken() + } + + fun String.tokenize() = sequence { + var firstLetter = -1 + var firstDigit = -1 + var openingBraceAt = -1 + var braceNestingLevel = 0 + + suspend fun SequenceScope.emitIdentifier(pos: Int = this@tokenize.length) { + assert(firstDigit < 0) + yield(this@tokenize.substring(firstLetter, pos)) + firstLetter = -1 + } + suspend fun SequenceScope.emitNumericLiteral(pos: Int = this@tokenize.length) { + assert(firstLetter < 0) + val token = this@tokenize.substring(firstDigit, pos) + if (token.toDoubleOrNull() == null) throw InvalidConstant(token) + yield(token) + firstDigit = -1 + } + + for ((pos, char) in this@tokenize.withIndex()) { + if (firstLetter >= 0) { + if (char.isIdentifierContinuation()) continue + emitIdentifier(pos) + } else if (firstDigit >= 0) { + if (char.isNumberLiteral()) continue + emitNumericLiteral(pos) + } + if (char.isWhitespace()) { + continue + } else if (openingBraceAt >= 0) { + if (char == '[') + braceNestingLevel++ + else if (char == ']') + braceNestingLevel-- + if (braceNestingLevel == 0) { + if (pos - openingBraceAt <= 1) throw EmptyBraces() + yield(this@tokenize.substring(openingBraceAt, pos + 1)) // Leave the braces + openingBraceAt = -1 + } + } else if (char.isIdentifierStart()) { + firstLetter = pos + continue + } else if (char.isNumberLiteral()) { + firstDigit = pos + continue + } else if (char == '[') { + openingBraceAt = pos + assert(braceNestingLevel == 0) + braceNestingLevel++ + } else if (char == ']') { + throw UnmatchedBraces(pos) + } else { + yield(char.toString()) + } + } + if (firstLetter >= 0) emitIdentifier() + if (firstDigit >= 0) emitNumericLiteral() + if (braceNestingLevel > 0) throw UnmatchedBraces(this@tokenize.length) + } +} diff --git a/tests/src/com/unciv/uniques/ExpressionTests.kt b/tests/src/com/unciv/uniques/ExpressionTests.kt new file mode 100644 index 0000000000000..bc0fbea505764 --- /dev/null +++ b/tests/src/com/unciv/uniques/ExpressionTests.kt @@ -0,0 +1,122 @@ +package com.unciv.uniques + +import com.badlogic.gdx.math.Vector2 +import com.unciv.models.ruleset.unique.expressions.Parser +import com.unciv.testing.GdxTestRunner +import com.unciv.testing.TestGame +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import kotlin.math.PI +import kotlin.math.abs +import kotlin.math.sin +import kotlin.math.ulp + +@RunWith(GdxTestRunner::class) +class ExpressionTests { + private val epsilon = 100.0.ulp + + @Test + fun testPrimitiveExpressions() { + val input = listOf( + ".98234792374" to .98234792374, + "4 - 2 + 4 + 30 + 6" to 42.0, + "2 + 4 * 10" to 42.0, + "2 * 4 + 10" to 18.0, + "42 / 7 / 2" to 3.0, + "42 / 2 / 7" to 3.0, + "666.66 % 7" to 666.66 % 7, + "42424 * -1 % 7" to 3.0, // true modulo, not kotlin's -4242.0 % 7 == -4 + "2 ^ 3 ^ 2" to 512.0, + "pi * .5" to PI / 2, + "(2+1.5)*(4+10)" to (2 + 1.5) * (4 + 10), + "sin(π/4)" to sin(PI / 4), + "sin π/4" to 0.0, + "+- -+-1" to -1.0, + ) + + var fails = 0 + for ((expression, expected) in input) { + val actual = try { + Parser.eval(expression) + } catch (_: Parser.ParsingError) { + null + } + if (actual != null && abs(actual - expected) < epsilon) continue + if (actual == null) + println("Expression \"$expression\" failed to evaluate, expected: $expected") + else { + println("AST: ${Parser.getASTDebugDescription(expression, null)}") + println("Expression \"$expression\" evaluated to $actual, expected: $expected") + } + fails++ + } + + assertEquals("failure count", 0, fails) + } + + @Test + fun testInvalidExpressions() { + val input = listOf( + "[fake countable]" to Parser.UnrecognizedToken::class, + "98.234.792.374" to Parser.InvalidConstant::class, + "" to Parser.MissingOperand::class, + "() - 2" to Parser.MissingOperand::class, + "((4 + 2) * 2" to Parser.UnmatchedParentheses::class, + "(3 + 9) % 2)" to Parser.UnmatchedParentheses::class, + "1 + []" to Parser.EmptyBraces::class, + "1 + [[Your] Cities]]" to Parser.UnmatchedBraces::class, + "[[[embarked] Units] + 1" to Parser.UnmatchedBraces::class, + ) + + var fails = 0 + for ((expression, expected) in input) { + var result: Exception? = null + try { + Parser.eval(expression) + } catch (ex: Exception) { + result = ex + } + if (result != null && expected.isInstance(result)) continue + if (result == null) + println("Expression \"$expression\" should throw ${expected.simpleName} but didn't") + else + println("Expression \"$expression\" threw ${result::class.simpleName}, expected: ${expected.simpleName}") + fails++ + } + + assertEquals("failure count", 0, fails) + } + + @Test + fun testExpressionsWithCountables() { + val game = TestGame() + game.makeHexagonalMap(2) + val civ = game.addCiv() + val city = game.addCity(civ, game.getTile(Vector2.Zero)) + + val input = listOf( + "√[[Your] Cities]" to 1.0, + "[Owned [worked] Tiles] / [Owned [unimproved] Tiles] * 100" to 100.0 / 6, // city center counts as improved + ) + + var fails = 0 + for ((expression, expected) in input) { + val actual = try { + Parser.eval(expression, game.ruleset, city.state) + } catch (_: Parser.ParsingError) { + null + } + if (actual != null && abs(actual - expected) < epsilon) continue + if (actual == null) + println("Expression \"$expression\" failed to evaluate, expected: $expected") + else { + println("AST: ${Parser.getASTDebugDescription(expression, game.ruleset)}") + println("Expression \"$expression\" evaluated to $actual, expected: $expected") + } + fails++ + } + + assertEquals("failure count", 0, fails) + } +} From dd46c401c472d0580c2787df8cb087e01ea00bbf Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Tue, 15 Apr 2025 23:12:27 +0200 Subject: [PATCH 06/11] Fix countable tests: correct expected failures --- tests/src/com/unciv/uniques/CountableTests.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/com/unciv/uniques/CountableTests.kt b/tests/src/com/unciv/uniques/CountableTests.kt index b516c0a84892c..f347141f22734 100644 --- a/tests/src/com/unciv/uniques/CountableTests.kt +++ b/tests/src/com/unciv/uniques/CountableTests.kt @@ -191,8 +191,8 @@ class CountableTests { "[+1 Happiness] " to 1, // +1 monkeys "[+1 Gold] " to 1, "[+1 Food] " to 0, - "[+1 Food] " to 2, - "[+1 Food] " to 3, + "[+1 Food] " to 1, // The Expression countable supports fractional numbers + "[+1 Food] " to 1, // dito "[+1 Food] " to 1, "[+1 Food] " to 0, "[+1 Food] " to 1, From 9b6161f695cfe82667ddfd1c021e5dd203a5f38a Mon Sep 17 00:00:00 2001 From: yairm210 Date: Thu, 17 Apr 2025 13:01:33 +0300 Subject: [PATCH 07/11] All syntax errors MUST indicate position. Otherwise, modders can't actually debug. --- .../ruleset/unique/expressions/Parser.kt | 45 ++++++++++------- .../ruleset/unique/expressions/Tokenizer.kt | 50 ++++++++++--------- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt index aac57fb737617..04dba503b2c77 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt @@ -45,19 +45,20 @@ object Parser { //region Exceptions /** Parent of all exceptions [parse] can throw. * If the exception caught is not [SyntaxError], then an Expression Countable should say NO "that can't possibly an expression". */ - open class ParsingError(override val message: String, open val severity: ICountable.MatchResult = ICountable.MatchResult.No) : Exception() + open class ParsingError(override val message: String, val position:Int, + open val severity: ICountable.MatchResult = ICountable.MatchResult.No) : Exception() /** Less severe than [ParsingError]. * It allows an Expression Countable to say "Maybe", meaning the string might be of type Expression, but malformed. */ - open class SyntaxError(message: String) : ParsingError(message, ICountable.MatchResult.Maybe) - class UnmatchedBraces(pos: Int) : ParsingError("Unmatched square braces at $pos") - class EmptyBraces : ParsingError("Empty square braces") - class UnmatchedParentheses(name: String) : SyntaxError("Unmatched $name parenthesis") - internal class UnexpectedToken(expected: Token, found: Token) : ParsingError("Unexpected token: $found instead of $expected") - class MissingOperand : SyntaxError("Missing operand") - class InvalidConstant(text: String) : SyntaxError("Invalid constant: $text") - class MalformedCountable(countable: Countables, text: String) : SyntaxError("Token \"$text\" seems to be a Countable(${countable.name}), but is malformed") - class UnrecognizedToken : ParsingError("Unrecognized token") - class InternalError : ParsingError("Internal error") + open class SyntaxError(message: String, position: Int) : ParsingError(message, position, ICountable.MatchResult.Maybe) + class UnmatchedBraces(position: Int) : ParsingError("Unmatched square braces", position) + class EmptyBraces(position: Int) : ParsingError("Empty square braces", position) + class UnmatchedParentheses(position: Int, name: String) : SyntaxError("Unmatched $name parenthesis", position) + internal class UnexpectedToken(position: Int, expected: Token, found: Token) : ParsingError("Unexpected token: $found instead of $expected", position) + class MissingOperand(position: Int) : SyntaxError("Missing operand", position) + class InvalidConstant(position: Int, text: String) : SyntaxError("Invalid constant: $text", position) + class MalformedCountable(position: Int, countable: Countables, text: String) : SyntaxError("Token \"$text\" seems to be a Countable(${countable.name}), but is malformed", position) + class UnrecognizedToken(position: Int) : ParsingError("Unrecognized token", position) + class InternalError(position: Int) : ParsingError("Internal error", position) //endregion /** Marker for beginning of the expression */ @@ -69,18 +70,19 @@ object Parser { override fun toString() = "end of expression" } - private class StateEngine(input: Sequence) { + private class StateEngine(input: Sequence>) { private var currentToken: Token = StartToken + private var currentPosition: Int = 0 private val iterator = input.iterator() private var openParenthesesCount = 0 private fun expect(expected: Token) { if (currentToken == expected) return if (expected == Parentheses.Closing && currentToken == EndToken) - throw UnmatchedParentheses(Parentheses.Opening.name.lowercase()) + throw UnmatchedParentheses(currentPosition, Parentheses.Opening.name.lowercase()) if (expected == EndToken && currentToken == Parentheses.Closing) - throw UnmatchedParentheses(Parentheses.Closing.name.lowercase()) - throw UnexpectedToken(expected, currentToken) + throw UnmatchedParentheses(currentPosition, Parentheses.Closing.name.lowercase()) + throw UnexpectedToken(currentPosition, expected, currentToken) } private fun next() { @@ -88,10 +90,17 @@ object Parser { openParenthesesCount++ } else if (currentToken == Parentheses.Closing) { if (openParenthesesCount == 0) - throw UnmatchedParentheses(Parentheses.Closing.name.lowercase()) + throw UnmatchedParentheses(currentPosition, Parentheses.Closing.name.lowercase()) openParenthesesCount-- } - currentToken = if (iterator.hasNext()) iterator.next() else EndToken + if (iterator.hasNext()){ + val (position, token) = iterator.next() + currentToken = token + currentPosition = position + } else { + currentToken = EndToken + // TODO: Not sure what to do about current position here + } } private fun handleUnary(): Node { @@ -127,7 +136,7 @@ object Parser { next() return node } else { - throw MissingOperand() + throw MissingOperand(currentPosition) } } diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt index fea3bcf4e57b0..5fb56fa438dfe 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt @@ -5,7 +5,6 @@ import com.unciv.models.ruleset.unique.Countables import com.unciv.models.ruleset.unique.ICountable import com.unciv.models.ruleset.unique.expressions.Operator.Parentheses import com.unciv.models.ruleset.unique.expressions.Parser.EmptyBraces -import com.unciv.models.ruleset.unique.expressions.Parser.InternalError import com.unciv.models.ruleset.unique.expressions.Parser.InvalidConstant import com.unciv.models.ruleset.unique.expressions.Parser.MalformedCountable import com.unciv.models.ruleset.unique.expressions.Parser.UnmatchedBraces @@ -40,40 +39,43 @@ internal object Tokenizer { private fun Char.isIdentifierStart() = isLetter() // Allow potentially non-latin script //TODO questionable private fun Char.isIdentifierContinuation() = this == '_' || isLetterOrDigit() - fun String.toToken(ruleset: Ruleset?): Token { - assert(isNotBlank()) - if (first().isDigit() || first() == '.') - return Node.NumericConstant(toDouble()) - val operator = Operator.of(this) - if (operator != null) return operator + // Position in text, to token found + fun Pair.toToken(ruleset: Ruleset?): Pair { + val (position, text) = this + if (text.isEmpty()) throw UnrecognizedToken(position) + assert(text.isNotBlank()) + if (text.first().isDigit() || text.first() == '.') + return position to Node.NumericConstant(text.toDouble()) + val operator = Operator.of(text) + if (operator != null) return position to operator // Countable tokens must come here still wrapped in braces to avoid infinite recursion - if(!startsWith('[') || !endsWith(']')) - throw UnrecognizedToken() - val countableText = substring(1, length - 1) + if(!text.startsWith('[') || !text.endsWith(']')) + throw UnrecognizedToken(position) + val countableText = text.substring(1, text.length - 1) val (countable, severity) = Countables.getBestMatching(countableText, ruleset) if (severity == ICountable.MatchResult.Yes) - return Node.Countable(countable!!, countableText) + return position to Node.Countable(countable!!, countableText) if (severity == ICountable.MatchResult.Maybe) - throw MalformedCountable(countable!!, countableText) - throw UnrecognizedToken() + throw MalformedCountable(position, countable!!, countableText) + throw UnrecognizedToken(position) } - fun String.tokenize() = sequence { + fun String.tokenize() = sequence> { var firstLetter = -1 var firstDigit = -1 var openingBraceAt = -1 var braceNestingLevel = 0 - suspend fun SequenceScope.emitIdentifier(pos: Int = this@tokenize.length) { + suspend fun SequenceScope>.emitIdentifier(pos: Int) { assert(firstDigit < 0) - yield(this@tokenize.substring(firstLetter, pos)) + yield(pos to this@tokenize.substring(firstLetter, pos)) firstLetter = -1 } - suspend fun SequenceScope.emitNumericLiteral(pos: Int = this@tokenize.length) { + suspend fun SequenceScope>.emitNumericLiteral(pos: Int) { assert(firstLetter < 0) val token = this@tokenize.substring(firstDigit, pos) - if (token.toDoubleOrNull() == null) throw InvalidConstant(token) - yield(token) + if (token.toDoubleOrNull() == null) throw InvalidConstant(pos, token) + yield(pos to token) firstDigit = -1 } @@ -93,8 +95,8 @@ internal object Tokenizer { else if (char == ']') braceNestingLevel-- if (braceNestingLevel == 0) { - if (pos - openingBraceAt <= 1) throw EmptyBraces() - yield(this@tokenize.substring(openingBraceAt, pos + 1)) // Leave the braces + if (pos - openingBraceAt <= 1) throw EmptyBraces(pos) + yield(pos to this@tokenize.substring(openingBraceAt, pos + 1)) // Leave the braces openingBraceAt = -1 } } else if (char.isIdentifierStart()) { @@ -110,11 +112,11 @@ internal object Tokenizer { } else if (char == ']') { throw UnmatchedBraces(pos) } else { - yield(char.toString()) + yield(pos to char.toString()) } } - if (firstLetter >= 0) emitIdentifier() - if (firstDigit >= 0) emitNumericLiteral() + if (firstLetter >= 0) emitIdentifier(this@tokenize.length) + if (firstDigit >= 0) emitNumericLiteral(this@tokenize.length) if (braceNestingLevel > 0) throw UnmatchedBraces(this@tokenize.length) } } From 4ad2cb72aa814cdb75da64f6b1d77fc7a41b8d27 Mon Sep 17 00:00:00 2001 From: yairm210 Date: Thu, 17 Apr 2025 13:19:21 +0300 Subject: [PATCH 08/11] Better positions + documentation --- .../ruleset/unique/expressions/Tokenizer.kt | 57 ++++++++++--------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt index 5fb56fa438dfe..0f61a3680a91d 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt @@ -44,12 +44,13 @@ internal object Tokenizer { val (position, text) = this if (text.isEmpty()) throw UnrecognizedToken(position) assert(text.isNotBlank()) - if (text.first().isDigit() || text.first() == '.') + if (text.first().isNumberLiteral()) return position to Node.NumericConstant(text.toDouble()) val operator = Operator.of(text) if (operator != null) return position to operator + // Countable tokens must come here still wrapped in braces to avoid infinite recursion - if(!text.startsWith('[') || !text.endsWith(']')) + if (!text.startsWith('[') || !text.endsWith(']')) throw UnrecognizedToken(position) val countableText = text.substring(1, text.length - 1) val (countable, severity) = Countables.getBestMatching(countableText, ruleset) @@ -61,52 +62,55 @@ internal object Tokenizer { } fun String.tokenize() = sequence> { - var firstLetter = -1 - var firstDigit = -1 - var openingBraceAt = -1 + /** If set, indicates we're in the middle of an identifier */ + var firstIdentifierPosition = -1 + /** If set, indicates we're in the middle of a number */ + var firstNumberPosition = -1 + /** If set, indicates we're in the middle of a countable */ + var openingBracePosition = -1 var braceNestingLevel = 0 suspend fun SequenceScope>.emitIdentifier(pos: Int) { - assert(firstDigit < 0) - yield(pos to this@tokenize.substring(firstLetter, pos)) - firstLetter = -1 + assert(firstNumberPosition < 0) + yield(firstIdentifierPosition to this@tokenize.substring(firstIdentifierPosition, pos)) + firstIdentifierPosition = -1 } suspend fun SequenceScope>.emitNumericLiteral(pos: Int) { - assert(firstLetter < 0) - val token = this@tokenize.substring(firstDigit, pos) - if (token.toDoubleOrNull() == null) throw InvalidConstant(pos, token) - yield(pos to token) - firstDigit = -1 + assert(firstIdentifierPosition < 0) + val token = this@tokenize.substring(firstNumberPosition, pos) + if (token.toDoubleOrNull() == null) throw InvalidConstant(firstNumberPosition, token) + yield(firstNumberPosition to token) + firstNumberPosition = -1 } for ((pos, char) in this@tokenize.withIndex()) { - if (firstLetter >= 0) { + if (firstIdentifierPosition >= 0) { if (char.isIdentifierContinuation()) continue emitIdentifier(pos) - } else if (firstDigit >= 0) { + } else if (firstNumberPosition >= 0) { if (char.isNumberLiteral()) continue emitNumericLiteral(pos) } - if (char.isWhitespace()) { - continue - } else if (openingBraceAt >= 0) { + if (char.isWhitespace()) continue + + if (openingBracePosition >= 0) { if (char == '[') braceNestingLevel++ else if (char == ']') braceNestingLevel-- if (braceNestingLevel == 0) { - if (pos - openingBraceAt <= 1) throw EmptyBraces(pos) - yield(pos to this@tokenize.substring(openingBraceAt, pos + 1)) // Leave the braces - openingBraceAt = -1 + if (pos - openingBracePosition <= 1) throw EmptyBraces(pos) + yield(pos to this@tokenize.substring(openingBracePosition, pos + 1)) // Leave the braces + openingBracePosition = -1 } } else if (char.isIdentifierStart()) { - firstLetter = pos + firstIdentifierPosition = pos continue } else if (char.isNumberLiteral()) { - firstDigit = pos + firstNumberPosition = pos continue } else if (char == '[') { - openingBraceAt = pos + openingBracePosition = pos assert(braceNestingLevel == 0) braceNestingLevel++ } else if (char == ']') { @@ -115,8 +119,9 @@ internal object Tokenizer { yield(pos to char.toString()) } } - if (firstLetter >= 0) emitIdentifier(this@tokenize.length) - if (firstDigit >= 0) emitNumericLiteral(this@tokenize.length) + // End of expression, let's see if there's still anything open + if (firstIdentifierPosition >= 0) emitIdentifier(this@tokenize.length) + if (firstNumberPosition >= 0) emitNumericLiteral(this@tokenize.length) if (braceNestingLevel > 0) throw UnmatchedBraces(this@tokenize.length) } } From b5bf57aba7cb7580032931e73e251cc8fbef2792 Mon Sep 17 00:00:00 2001 From: yairm210 Date: Thu, 17 Apr 2025 14:29:02 +0300 Subject: [PATCH 09/11] Get rid of functions that modders *should not* be using --- .../ruleset/unique/expressions/Operator.kt | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt index f927d83d524e2..34f06b2655aae 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Operator.kt @@ -1,16 +1,6 @@ package com.unciv.models.ruleset.unique.expressions -import kotlin.math.abs -import kotlin.math.atan -import kotlin.math.cbrt -import kotlin.math.cos -import kotlin.math.exp -import kotlin.math.ln -import kotlin.math.log10 -import kotlin.math.pow -import kotlin.math.sin -import kotlin.math.sqrt -import kotlin.math.tan +import kotlin.math.* internal sealed interface Operator : Tokenizer.Token { val symbol: String @@ -39,17 +29,11 @@ internal sealed interface Operator : Tokenizer.Token { ) : Unary { Identity("+", { operand -> operand }), Negation("-", { operand -> -operand }), - Sqrt("√", ::sqrt), + Ciel("√", ::sqrt), Abs("abs", ::abs), Sqrt2("sqrt", ::sqrt), - Cbrt("cbtr", ::cbrt), - Exp("exp", ::exp), - Log("ln", ::ln), - Log10("log10", ::log10), - Sin("sin", ::sin), - Cos("cos", ::cos), - Tan("tan", ::tan), - Atan("atan", ::atan), + Floor("floor", ::floor), + Ceil("ceil", ::ceil), ; override fun toString() = symbol } From d52c2771b41835857ba1c24214e66b6d2e561c14 Mon Sep 17 00:00:00 2001 From: yairm210 Date: Thu, 17 Apr 2025 14:40:59 +0300 Subject: [PATCH 10/11] Fix tests given the new changes --- .../unciv/models/ruleset/unique/expressions/Parser.kt | 7 ++++--- .../models/ruleset/unique/expressions/Tokenizer.kt | 11 +++++++---- tests/src/com/unciv/uniques/ExpressionTests.kt | 6 ++---- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt index 04dba503b2c77..108dff77442e2 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Parser.kt @@ -56,9 +56,10 @@ object Parser { internal class UnexpectedToken(position: Int, expected: Token, found: Token) : ParsingError("Unexpected token: $found instead of $expected", position) class MissingOperand(position: Int) : SyntaxError("Missing operand", position) class InvalidConstant(position: Int, text: String) : SyntaxError("Invalid constant: $text", position) - class MalformedCountable(position: Int, countable: Countables, text: String) : SyntaxError("Token \"$text\" seems to be a Countable(${countable.name}), but is malformed", position) - class UnrecognizedToken(position: Int) : ParsingError("Unrecognized token", position) - class InternalError(position: Int) : ParsingError("Internal error", position) + class MalformedCountable(position: Int, countable: Countables, text: String) : SyntaxError("\"$text\" seems to be a Countable(${countable.name}), but is malformed", position) + class UnknownCountable(position: Int, text: String) : ParsingError("Unknown countable: \"$text\"", position) + class UnknownIdentifier(position: Int, text: String) : ParsingError("Unknown identifier: \"$text\"", position) + class EmptyExpression : ParsingError("Empty expression", 0) //endregion /** Marker for beginning of the expression */ diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt index 0f61a3680a91d..0b5f46c0eebb0 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Tokenizer.kt @@ -6,9 +6,11 @@ import com.unciv.models.ruleset.unique.ICountable import com.unciv.models.ruleset.unique.expressions.Operator.Parentheses import com.unciv.models.ruleset.unique.expressions.Parser.EmptyBraces import com.unciv.models.ruleset.unique.expressions.Parser.InvalidConstant +import com.unciv.models.ruleset.unique.expressions.Parser.UnknownCountable import com.unciv.models.ruleset.unique.expressions.Parser.MalformedCountable import com.unciv.models.ruleset.unique.expressions.Parser.UnmatchedBraces -import com.unciv.models.ruleset.unique.expressions.Parser.UnrecognizedToken +import com.unciv.models.ruleset.unique.expressions.Parser.UnknownIdentifier +import com.unciv.models.ruleset.unique.expressions.Parser.EmptyExpression internal object Tokenizer { /** @@ -42,7 +44,7 @@ internal object Tokenizer { // Position in text, to token found fun Pair.toToken(ruleset: Ruleset?): Pair { val (position, text) = this - if (text.isEmpty()) throw UnrecognizedToken(position) + if (text.isEmpty()) throw EmptyExpression() assert(text.isNotBlank()) if (text.first().isNumberLiteral()) return position to Node.NumericConstant(text.toDouble()) @@ -51,14 +53,15 @@ internal object Tokenizer { // Countable tokens must come here still wrapped in braces to avoid infinite recursion if (!text.startsWith('[') || !text.endsWith(']')) - throw UnrecognizedToken(position) + throw UnknownIdentifier(position, text) + val countableText = text.substring(1, text.length - 1) val (countable, severity) = Countables.getBestMatching(countableText, ruleset) if (severity == ICountable.MatchResult.Yes) return position to Node.Countable(countable!!, countableText) if (severity == ICountable.MatchResult.Maybe) throw MalformedCountable(position, countable!!, countableText) - throw UnrecognizedToken(position) + throw UnknownCountable(position, text) } fun String.tokenize() = sequence> { diff --git a/tests/src/com/unciv/uniques/ExpressionTests.kt b/tests/src/com/unciv/uniques/ExpressionTests.kt index bc0fbea505764..36501d55f1706 100644 --- a/tests/src/com/unciv/uniques/ExpressionTests.kt +++ b/tests/src/com/unciv/uniques/ExpressionTests.kt @@ -9,7 +9,6 @@ import org.junit.Test import org.junit.runner.RunWith import kotlin.math.PI import kotlin.math.abs -import kotlin.math.sin import kotlin.math.ulp @RunWith(GdxTestRunner::class) @@ -30,8 +29,6 @@ class ExpressionTests { "2 ^ 3 ^ 2" to 512.0, "pi * .5" to PI / 2, "(2+1.5)*(4+10)" to (2 + 1.5) * (4 + 10), - "sin(π/4)" to sin(PI / 4), - "sin π/4" to 0.0, "+- -+-1" to -1.0, ) @@ -58,7 +55,8 @@ class ExpressionTests { @Test fun testInvalidExpressions() { val input = listOf( - "[fake countable]" to Parser.UnrecognizedToken::class, + "fake_function(2)" to Parser.UnknownIdentifier::class, + "[fake countable]" to Parser.UnknownCountable::class, "98.234.792.374" to Parser.InvalidConstant::class, "" to Parser.MissingOperand::class, "() - 2" to Parser.MissingOperand::class, From 442600c53f781e4c5f2d0447ad10dd1996d2e2cf Mon Sep 17 00:00:00 2001 From: yairm210 Date: Thu, 17 Apr 2025 15:30:27 +0300 Subject: [PATCH 11/11] Add modder-visible expression parsing errors --- .../unciv/models/ruleset/unique/Countables.kt | 32 +++++++++---------- .../ruleset/unique/expressions/Expressions.kt | 9 ++++-- .../ruleset/validation/UniqueValidator.kt | 24 ++++++++++++++ docs/Modders/Unique-parameters.md | 4 +++ 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index 50744d81e0698..50c3f916bc31d 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -26,19 +26,13 @@ import java.time.format.DateTimeFormatter interface ICountable { /** Supports `MatchResult(true)`to get [Yes], MatchResult(false)`to get [No], or MatchResult(null)`to get [Maybe] */ enum class MatchResult { - No { - override fun isOK(strict: Boolean) = false - }, - Maybe { - override fun isOK(strict: Boolean) = !strict - }, - Yes { - override fun isOK(strict: Boolean) = true - } + No { override fun isOK(strict: Boolean) = false }, + Maybe { override fun isOK(strict: Boolean) = !strict }, + Yes { override fun isOK(strict: Boolean) = true } ; abstract fun isOK(strict: Boolean): Boolean companion object { - operator fun invoke(bool: Boolean?) = when(bool) { + fun from(bool: Boolean?) = when(bool) { true -> Yes false -> No else -> Maybe @@ -79,7 +73,7 @@ enum class Countables( ) : ICountable { Integer { override val documentationHeader = "Integer constant - any positive or negative integer number" - override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(parameterText.toIntOrNull() != null) + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult.from(parameterText.toIntOrNull() != null) override fun eval(parameterText: String, stateForConditionals: StateForConditionals) = parameterText.toIntOrNull() }, @@ -105,7 +99,11 @@ enum class Countables( Stats { override val documentationHeader = "Stat name (${Stat.names().niceJoin()})" override val documentationStrings = listOf("gets the stat *reserve*, not the amount per turn (can be city stats or civilization stats, depending on where the unique is used)") - override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(Stat.isStat(parameterText)) + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult.from( + Stat.isStat( + parameterText + ) + ) override fun eval(parameterText: String, stateForConditionals: StateForConditionals): Int? { val relevantStat = Stat.safeValueOf(parameterText) ?: return null // This one isn't covered by City.getStatReserve or Civilization.getStatReserve but should be available here @@ -189,7 +187,9 @@ enum class Countables( "For example: If a unique is placed on a building, then the retrieved resources will be of the city. If placed on a policy, they will be of the civilization.", "This can make a difference for e.g. local resources, which are counted per city." ) - override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult(ruleset?.tileResources?.containsKey(parameterText)) + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult.from( + ruleset?.tileResources?.containsKey(parameterText) + ) override fun eval(parameterText: String, stateForConditionals: StateForConditionals) = stateForConditionals.getResourceAmount(parameterText) override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = ruleset.tileResources.keys @@ -204,7 +204,6 @@ enum class Countables( } }, - @InDevelopment("@AutumnPizazz", eta = "2025-06-30") Expression { override val noPlaceholders = false @@ -221,7 +220,7 @@ enum class Countables( override val documentationHeader = "Evaluate expressions!" override val documentationStrings = listOf( - "Expressions support `+`, `-`, `*`, `/`, `%`, `^` and `log` as binary operators.", + "Expressions support `+`, `-`, `*`, `/`, `%`, `^` operations.", "Operands can be floating point constants or other countables in square brackets", "..." ) @@ -235,7 +234,7 @@ enum class Countables( open val noPlaceholders = !text.contains('[') // Leave these in place only for the really simple cases - override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult( + override fun matches(parameterText: String, ruleset: Ruleset?) = ICountable.MatchResult.from( if (noPlaceholders) parameterText == text else parameterText.equalsPlaceholderText(placeholderText) ) @@ -268,7 +267,6 @@ enum class Countables( * - If you add both @Deprecated and @InDevelopment to the same instance, the validator will only see the @Deprecated one, * while @InDevelopment still controls whether it si active (evaluated at all). * @param developer - the responsible dev as `@` plus github account - * @param eta - An expiration date in the form 'yyyy-mm-dd', UTC, after which the instance is treated as deprecated **and** inactive */ @Retention(AnnotationRetention.RUNTIME) @Target(AnnotationTarget.FIELD) diff --git a/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt b/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt index 6a3f919ed98ba..e8a18e55c050b 100644 --- a/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt +++ b/core/src/com/unciv/models/ruleset/unique/expressions/Expressions.kt @@ -22,7 +22,7 @@ class Expressions : ICountable { ICountable.MatchResult.Yes -> null } - private data class ParseResult(val severity: ICountable.MatchResult, val node: Node?) + private data class ParseResult(val severity: ICountable.MatchResult, val node: Node?, val exception: Parser.ParsingError?) companion object { private val cache: MutableMap = mutableMapOf() @@ -30,10 +30,13 @@ class Expressions : ICountable { private fun parse(parameterText: String, ruleset: Ruleset?): ParseResult = cache.getOrPut(parameterText) { try { val node = Parser.parse(parameterText, ruleset) - ParseResult(ICountable.MatchResult.Yes, node) + ParseResult(ICountable.MatchResult.Yes, node, null) } catch (ex: Parser.ParsingError) { - ParseResult(ex.severity, null) + ParseResult(ex.severity, null, ex) } } + + fun getParsingError(parameterText: String, ruleset: Ruleset?): Parser.ParsingError? = + parse(parameterText, ruleset).exception } } diff --git a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt index 7a9dfb31b8235..e2c5489fe94c4 100644 --- a/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt +++ b/core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt @@ -14,6 +14,7 @@ import com.unciv.models.ruleset.unique.UniqueFlag import com.unciv.models.ruleset.unique.UniqueParameterType import com.unciv.models.ruleset.unique.UniqueTarget import com.unciv.models.ruleset.unique.UniqueType +import com.unciv.models.ruleset.unique.expressions.Expressions class UniqueValidator(val ruleset: Ruleset) { @@ -85,6 +86,8 @@ class UniqueValidator(val ruleset: Ruleset) { " ${complianceError.acceptableParameterTypes.joinToString(" or ") { it.parameterName }} !", complianceError.errorSeverity.getRulesetErrorSeverity(), uniqueContainer, unique ) + + addExpressionParseErrors(complianceError, rulesetErrors, uniqueContainer, unique) } for (conditional in unique.modifiers) { @@ -124,6 +127,25 @@ class UniqueValidator(val ruleset: Ruleset) { return rulesetErrors } + private fun addExpressionParseErrors( + complianceError: UniqueComplianceError, + rulesetErrors: RulesetErrorList, + uniqueContainer: IHasUniques?, + unique: Unique + ) { + if (!complianceError.acceptableParameterTypes.contains(UniqueParameterType.Countable)) return + val parseError = Expressions.getParsingError(complianceError.parameterName, ruleset) ?: return + + val marker = "HERE➡" + val errorLocation = parseError.position + val parameterWithErrorLocationMarked = + complianceError.parameterName.substring(0, errorLocation) + marker + + complianceError.parameterName.substring(errorLocation) + val text = "\"${complianceError.parameterName}\" could not be parsed as an expression due to:" + + " ${parseError.message}. \n$parameterWithErrorLocationMarked" + rulesetErrors.add(text, RulesetErrorSeverity.Error, uniqueContainer, unique) + } + private val resourceUniques = setOf(UniqueType.ProvidesResources, UniqueType.ConsumesResources, UniqueType.DoubleResourceProduced, UniqueType.StrategicResourcesIncrease) private val resourceConditionals = setOf( @@ -221,6 +243,8 @@ class UniqueValidator(val ruleset: Ruleset) { " ${complianceError.acceptableParameterTypes.joinToString(" or ") { it.parameterName }} !", complianceError.errorSeverity.getRulesetErrorSeverity(), uniqueContainer, unique ) + + addExpressionParseErrors(complianceError, rulesetErrors, uniqueContainer, unique) } addDeprecationAnnotationErrors(conditional, "$prefix contains modifier \"${conditional.text}\" which", rulesetErrors, uniqueContainer) diff --git a/docs/Modders/Unique-parameters.md b/docs/Modders/Unique-parameters.md index 35b9da7225332..9e54def3523b0 100644 --- a/docs/Modders/Unique-parameters.md +++ b/docs/Modders/Unique-parameters.md @@ -359,5 +359,9 @@ Allowed values: (can be city stats or civilization stats, depending on where the unique is used) For example: If a unique is placed on a building, then the retrieved resources will be of the city. If placed on a policy, they will be of the civilization. This can make a difference for e.g. local resources, which are counted per city. +- Evaluate expressions! + Expressions support `+`, `-`, `*`, `/`, `%`, `^` operations. + Operands can be floating point constants or other countables in square brackets + ... [//]: # (Countables automatically generated END)