diff --git a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/ConvertedEventValue.java b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/ConvertedEventValue.java index 67da8402903..050dc482340 100644 --- a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/ConvertedEventValue.java +++ b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/ConvertedEventValue.java @@ -191,6 +191,11 @@ public Time time() { return source.excludedErrorMessage(); } + @Override + public boolean contextDependent() { + return source.contextDependent(); + } + @Override public boolean matches(EventValue eventValue) { return matches(eventValue.eventClass(), eventValue.valueClass(), eventValue.patterns()); diff --git a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValue.java b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValue.java index 870fc4a054a..c8a4362566e 100644 --- a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValue.java +++ b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValue.java @@ -155,6 +155,21 @@ static EventValue simple(Class eventClass, Class + * Defaults to {@code false}; only set this when an {@link Builder#eventValidator(Function) + * event validator} is not pure for a given event class. + * + * @return {@code true} if resolutions involving this value should bypass the cache + */ + @Contract(pure = true) + boolean contextDependent(); + /** * Checks whether this event value matches the provided event value in terms of * event class, value class, and identifier patterns. @@ -453,6 +468,18 @@ default Builder excludes(Class event1, Class eve @Contract(value = "_ -> this", mutates = "this") Builder excludedErrorMessage(String excludedErrorMessage); + /** + * Marks this event value as context-dependent. Resolutions that consider this value will + * not be cached, so the {@linkplain #eventValidator(Function) event validator} is + * consulted on every lookup. Use this when the validator's answer for a given event + * class may change at runtime (e.g. because it consults external state). + * + * @return this builder + * @see EventValue#contextDependent() + */ + @Contract(value = " -> this", mutates = "this") + Builder contextDependent(); + /** * Builds the event value. * diff --git a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueImpl.java b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueImpl.java index b9e91c4a7d9..f462ff5eb4d 100644 --- a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueImpl.java +++ b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueImpl.java @@ -36,6 +36,7 @@ final class EventValueImpl implements EventValue { private final Time time; private final Collection> excludedEvents; private final @Nullable String excludedErrorMessage; + private final boolean contextDepenent; private SkriptPattern[] compiledPatterns; @@ -51,6 +52,7 @@ private EventValueImpl(BuilderImpl builder) { this.time = builder.time; this.excludedEvents = builder.excludedEvents; this.excludedErrorMessage = builder.excludedErrorMessage; + this.contextDepenent = builder.contextDependent; } @Override @@ -153,6 +155,11 @@ public Time time() { return excludedErrorMessage; } + @Override + public boolean contextDependent() { + return contextDepenent; + } + @Override public boolean matches(EventValue eventValue) { return matches(eventValue.eventClass(), eventValue.valueClass(), eventValue.patterns()) @@ -204,6 +211,7 @@ static class BuilderImpl implements Builder { private Time time = Time.NOW; private Collection> excludedEvents = Collections.emptyList(); private @Nullable String excludedErrorMessage; + private boolean contextDependent = false; BuilderImpl(Class eventClass, Class valueClass) { this.eventClass = eventClass; @@ -260,6 +268,12 @@ public Builder excludedErrorMessage(String excludedErrorMessage) { return this; } + @Override + public Builder contextDependent() { + this.contextDependent = true; + return this; + } + @Override public EventValue build() { if (patterns == null) { diff --git a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryImpl.java b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryImpl.java index 152dec37b1e..8e641044cad 100644 --- a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryImpl.java +++ b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryImpl.java @@ -66,6 +66,12 @@ public boolean isRegistered(Class eventClass, Class valueCla return false; } + private void cache(Input input, Resolution resolution, boolean contextDependent) { + if (contextDependent) + return; + eventValuesCache.put(input, resolution); + } + @Override public Resolution resolve(Class eventClass, String identifier) { return resolve(eventClass, identifier, EventValue.Time.NOW); @@ -96,14 +102,15 @@ public Resolution resolve( return resolution; //noinspection unchecked - resolution = Resolver.builder(eventClass) + var output = Resolver.builder(eventClass) .filter(ev -> ClassUtils.isRelatedTo(ev.eventClass(), eventClass) && ev.matchesInput(identifier)) .comparator(Resolver.EVENT_DISTANCE_COMPARATOR) .mapper(ev -> (EventValue) ev.getConverted(eventClass, ev.valueClass())) .build().resolve(eventValues(time)); + resolution = output.resolution(); - if (resolution.successful()) { - eventValuesCache.put(input, resolution); + if (resolution.successful() || resolution.errored()) { + cache(input, resolution, output.contextDependent()); return resolution; } @@ -111,7 +118,7 @@ public Resolution resolve( return resolve(eventClass, identifier, EventValue.Time.NOW, flags); resolution = Resolution.empty(); - eventValuesCache.put(input, resolution); + cache(input, resolution, output.contextDependent()); return resolution; } @@ -144,31 +151,36 @@ public Resolution resolve( if (resolution != null) return resolution; - resolution = resolveExact(eventClass, valueClass, time) - .anyOptional() - .map(eventValue -> Resolution.of(Collections.singletonList(eventValue))) - .orElse(Resolution.empty()); + var output = Resolver.exact(eventClass, valueClass).resolve(eventValues(time)); + resolution = output.resolution(); + if (resolution.successful() || resolution.errored()) { - eventValuesCache.put(input, resolution); + cache(input, resolution, output.contextDependent()); return resolution; } - resolution = resolveNearest(eventClass, valueClass, time); + output = resolveNearest(eventClass, valueClass, time); + resolution = output.resolution(); + if (resolution.successful() || resolution.errored()) { - eventValuesCache.put(input, resolution); + cache(input, resolution, output.contextDependent()); return resolution; } if (flags.has(Flag.ALLOW_CONVERSION)) { - resolution = resolveWithDowncastConversion(eventClass, valueClass, time); + output = resolveWithDowncastConversion(eventClass, valueClass, time); + resolution = output.resolution(); + if (resolution.successful() || resolution.errored()) { - eventValuesCache.put(input, resolution); + cache(input, resolution, output.contextDependent()); return resolution; } - resolution = resolveWithConversion(eventClass, valueClass, time); + output = resolveWithConversion(eventClass, valueClass, time); + resolution = output.resolution(); + if (resolution.successful() || resolution.errored()) { - eventValuesCache.put(input, resolution); + cache(input, resolution, output.contextDependent()); return resolution; } } @@ -177,7 +189,6 @@ public Resolution resolve( return resolve(eventClass, valueClass, EventValue.Time.NOW, flags); resolution = Resolution.empty(); - eventValuesCache.put(input, resolution); return resolution; } @@ -187,17 +198,13 @@ public Resolution resolveExact( Class valueClass, EventValue.Time time ) { - return Resolver.builder(eventClass, valueClass) - .filter(ev -> ev.eventClass().isAssignableFrom(eventClass) && ev.valueClass().equals(valueClass)) - .comparator(Resolver.EVENT_DISTANCE_COMPARATOR) - .filterMatches() - .build().resolve(eventValues(time)); + return Resolver.exact(eventClass, valueClass).resolve(eventValues(time)).resolution(); } /** * Resolves to the nearest event and value class without conversion. */ - private Resolution resolveNearest( + private Resolver.Output resolveNearest( Class eventClass, Class valueClass, EventValue.Time time @@ -214,7 +221,7 @@ public Resolution resolveExact( * Resolves using downcast conversion when the desired value class is a supertype * of the registered value class. */ - private Resolution resolveWithDowncastConversion( + private Resolver.Output resolveWithDowncastConversion( Class eventClass, Class valueClass, EventValue.Time time @@ -233,7 +240,7 @@ private Resolution resolveWithDowncastConversion( /** * Resolves using {@link Converters} to convert value type when needed. */ - private Resolution resolveWithConversion( + private Resolver.Output resolveWithConversion( Class eventClass, Class valueClass, EventValue.Time time diff --git a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/Resolver.java b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/Resolver.java index 1865e6de5af..254a4320fb6 100644 --- a/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/Resolver.java +++ b/src/main/java/org/skriptlang/skript/bukkit/lang/eventvalue/Resolver.java @@ -74,21 +74,25 @@ private Resolver( * Resolves the given list of {@link EventValue}s. * * @param eventValues The event values to resolve. - * @return The resolution containing the best candidates. + * @return The resolution containing the best candidates, along with a flag indicating + * whether any candidate considered was {@linkplain EventValue#contextDependent() context-dependent}. */ - public EventValueRegistry.Resolution resolve(List> eventValues) { + public Output resolve(List> eventValues) { List> best = new ArrayList<>(); EventValue bestMatch = null; + boolean contextDependent = false; + for (EventValue eventValue : eventValues) { if (!filter.test(eventValue)) continue; + contextDependent |= eventValue.contextDependent(); switch (eventValue.validate(eventClass)) { case INVALID -> { continue; } case ABORT -> { - return EventValueRegistry.Resolution.error(); + return new Output<>(EventValueRegistry.Resolution.error(), contextDependent); } } @@ -109,9 +113,10 @@ public EventValueRegistry.Resolution resolve(List> eventV best.add(converted); } } + if (valueClass != null && filterMatches) - return EventValueRegistry.Resolution.of(filterEventValues(valueClass, best)); - return EventValueRegistry.Resolution.of(best); + return new Output<>(EventValueRegistry.Resolution.of(filterEventValues(valueClass, best)), contextDependent); + return new Output<>(EventValueRegistry.Resolution.of(best), contextDependent); } /** @@ -173,6 +178,14 @@ static Builder builder(Class eventClass, Class return new Builder<>(eventClass, valueClass); } + static Resolver exact(Class eventClass, Class valueClass) { + return Resolver.builder(eventClass, valueClass) + .filter(ev -> ev.eventClass().isAssignableFrom(eventClass) && ev.valueClass().equals(valueClass)) + .comparator(Resolver.EVENT_DISTANCE_COMPARATOR) + .filterMatches() + .build(); + } + /** * A builder for {@link Resolver}. * @@ -309,4 +322,19 @@ interface EventValueComparatorFactory { } + /** + * The result of a {@link Resolver#resolve(List) resolve} call. + * + * @param resolution The resolution produced. + * @param contextDependent Whether any candidate considered during resolution was + * {@linkplain EventValue#contextDependent() context-dependent}, + * meaning the result must not be cached. + * @param The event type. + * @param The value type. + */ + record Output( + EventValueRegistry.Resolution resolution, + boolean contextDependent + ) {} + } diff --git a/src/test/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryTest.java b/src/test/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryTest.java new file mode 100644 index 00000000000..30c80b2eb70 --- /dev/null +++ b/src/test/java/org/skriptlang/skript/bukkit/lang/eventvalue/EventValueRegistryTest.java @@ -0,0 +1,262 @@ +package org.skriptlang.skript.bukkit.lang.eventvalue; + +import org.bukkit.event.Event; +import org.bukkit.event.HandlerList; +import org.jetbrains.annotations.NotNull; +import org.junit.Before; +import org.junit.Test; +import org.skriptlang.skript.lang.converter.Converters; + +import static org.junit.Assert.*; + +public class EventValueRegistryTest { + + private EventValueRegistry registry; + + @Before + public void setUp() { + registry = EventValueRegistry.empty(null); + } + + private static class TestEvent extends Event { + @Override + public @NotNull HandlerList getHandlers() { + throw new UnsupportedOperationException(); + } + } + private static class SubTestEvent extends TestEvent {} + private static class OtherEvent extends Event { + @Override + public @NotNull HandlerList getHandlers() { + throw new UnsupportedOperationException(); + } + } + + private static class TestValue {} + private static class SubTestValue extends TestValue {} + private static class OtherValue {} + + @Test + public void testRegistration() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .getter(e -> new TestValue()) + .build(); + + assertFalse(registry.isRegistered(ev)); + registry.register(ev); + assertTrue(registry.isRegistered(ev)); + assertTrue(registry.isRegistered(TestEvent.class, TestValue.class, EventValue.Time.NOW)); + + assertTrue(registry.elements().contains(ev)); + assertTrue(registry.elements(EventValue.Time.NOW).contains(ev)); + assertTrue(registry.elements(TestEvent.class).contains(ev)); + + assertTrue(registry.unregister(ev)); + assertFalse(registry.isRegistered(ev)); + assertFalse(registry.elements().contains(ev)); + } + + @Test + public void testResolveByIdentifier() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test pattern") + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + EventValueRegistry.Resolution res = registry.resolve(TestEvent.class, "test pattern"); + assertTrue(res.successful()); + assertEquals(ev, res.unique()); + + // Test sub-event + EventValueRegistry.Resolution subRes = registry.resolve(SubTestEvent.class, "test pattern"); + assertTrue(subRes.successful()); + // It should be a converted event value + assertEquals(ev.valueClass(), subRes.unique().valueClass()); + assertEquals(TestEvent.class, subRes.unique().eventClass()); + + // Test non-matching identifier + assertFalse(registry.resolve(TestEvent.class, "wrong").successful()); + } + + @Test + public void testResolveByValueClass() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + EventValueRegistry.Resolution res = registry.resolve(TestEvent.class, TestValue.class); + assertTrue(res.successful()); + assertEquals(ev, res.unique()); + + EventValueRegistry.Resolution subValueRes = registry.resolve(TestEvent.class, SubTestValue.class); + assertTrue(subValueRes.successful()); + } + + @Test + public void testCacheLogic() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + EventValueRegistry.Resolution res1 = registry.resolve(TestEvent.class, "test"); + EventValueRegistry.Resolution res2 = registry.resolve(TestEvent.class, "test"); + + assertSame("Resolutions should be cached and return the same instance", res1, res2); + + // Registering a new value should clear the cache + EventValue ev2 = EventValue.builder(OtherEvent.class, TestValue.class) + .patterns("other") + .getter(e -> new TestValue()) + .build(); + registry.register(ev2); + + EventValueRegistry.Resolution res3 = registry.resolve(TestEvent.class, "test"); + assertNotSame("Cache should have been cleared", res1, res3); + assertEquals(res1, res3); + } + + @Test + public void testContextDependentNotCached() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .getter(e -> new TestValue()) + .contextDependent() + .build(); + registry.register(ev); + + EventValueRegistry.Resolution res1 = registry.resolve(TestEvent.class, "test"); + EventValueRegistry.Resolution res2 = registry.resolve(TestEvent.class, "test"); + + assertNotSame("Context-dependent resolutions should not be cached", res1, res2); + assertEquals(res1, res2); + } + + @Test + public void testUnmodifiableView() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + EventValueRegistry unmodifiable = registry.unmodifiableView(); + assertTrue(unmodifiable.isRegistered(ev)); + + assertThrows(UnsupportedOperationException.class, () -> unmodifiable.register(ev)); + assertThrows(UnsupportedOperationException.class, () -> unmodifiable.unregister(ev)); + } + + @Test + public void testTimeFallback() { + EventValue evPast = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .time(EventValue.Time.PAST) + .getter(e -> new TestValue()) + .build(); + registry.register(evPast); + + // Resolve past + assertTrue(registry.resolve(TestEvent.class, "test", EventValue.Time.PAST).successful()); + // Resolve now (should fail) + assertFalse(registry.resolve(TestEvent.class, "test", EventValue.Time.NOW).successful()); + + EventValue evNow = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .time(EventValue.Time.NOW) + .getter(e -> new TestValue()) + .build(); + registry.register(evNow); + + // Resolve with fallback + EventValueRegistry.Resolution res = registry.resolve( + TestEvent.class, "test", EventValue.Time.FUTURE, EventValueRegistry.Flags.of(EventValueRegistry.Flag.FALLBACK_TO_DEFAULT_TIME_STATE)); + assertTrue(res.successful()); + assertEquals(evNow.valueClass(), res.unique().valueClass()); + assertEquals(EventValue.Time.NOW, res.unique().time()); + } + + @Test + public void testResolveExact() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + assertTrue(registry.resolveExact(TestEvent.class, TestValue.class, EventValue.Time.NOW).successful()); + assertTrue(registry.resolveExact(SubTestEvent.class, TestValue.class, EventValue.Time.NOW).successful()); + assertFalse(registry.resolveExact(TestEvent.class, SubTestEvent.class, EventValue.Time.NOW).successful()); + } + + @Test + public void testAmbiguousResolution() { + EventValue ev1 = EventValue.builder(TestEvent.class, Integer.class) + .patterns("test") + .getter(e -> 10) + .build(); + EventValue ev2 = EventValue.builder(TestEvent.class, Double.class) + .patterns("test") + .getter(e -> 10.0) + .build(); + registry.register(ev1); + registry.register(ev2); + + EventValueRegistry.Resolution res = registry.resolve(TestEvent.class, "test"); + assertTrue(res.successful()); + assertTrue(res.multiple()); + assertEquals(2, res.size()); + assertNotNull(res.any()); + assertTrue(res.anyOptional().isPresent()); + assertThrows(IllegalStateException.class, res::unique); + } + + @Test + public void testAbortValidation() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .patterns("test") + .eventValidator(event -> event.equals(SubTestEvent.class) ? EventValue.Validation.ABORT : EventValue.Validation.VALID) + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + assertTrue(registry.resolve(TestEvent.class, "test").successful()); + EventValueRegistry.Resolution res = registry.resolve(SubTestEvent.class, "test"); + assertFalse(res.successful()); + assertTrue(res.errored()); + } + + @Test + public void testDowncastConversion() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + // Resolve for SubTestValue (subtype of TestValue) should work with ALLOW_CONVERSION + EventValueRegistry.Resolution res = registry.resolve(TestEvent.class, SubTestValue.class, EventValue.Time.NOW, + EventValueRegistry.Flags.of(EventValueRegistry.Flag.ALLOW_CONVERSION)); + assertTrue(res.successful()); + assertEquals(SubTestValue.class, res.unique().valueClass()); + } + + @Test + public void testConverter() { + EventValue ev = EventValue.builder(TestEvent.class, TestValue.class) + .getter(e -> new TestValue()) + .build(); + registry.register(ev); + + // Register a converter from TestValue to OtherValue + Converters.registerConverter(TestValue.class, OtherValue.class, from -> new OtherValue()); + + // Resolve for OtherValue should work with ALLOW_CONVERSION + EventValueRegistry.Resolution res = registry.resolve(TestEvent.class, OtherValue.class, EventValue.Time.NOW, + EventValueRegistry.Flags.of(EventValueRegistry.Flag.ALLOW_CONVERSION)); + assertTrue(res.successful()); + assertEquals(OtherValue.class, res.unique().valueClass()); + } +}