Skip to content

Commit

Permalink
Limit the number of reported warnings (#6577)
Browse files Browse the repository at this point in the history
Artifically limiting the number of reported warnings to 100. Also added benchmarks with random Ints to investigate perf issues when dealing with warnings (future task).
Ideally we would have a custom set-like collection that allows us internally to specify a maximal number of elements. But `EnsoHashMap` (and potentially `EnsoSet`) are still WIP when it comes to being PE-friendly.

The change also allows for checking if the limit for the number of reported warnings has been reached. It will visualize by adding an additional "Warnings limit reached." to the visualization.

The limit is configurable via `--warnings-limit` parameter to `run`.

Closes #6283.
  • Loading branch information
hubertp authored May 10, 2023
1 parent 9597dd3 commit cf3624c
Show file tree
Hide file tree
Showing 34 changed files with 385 additions and 91 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@
finalizers][6335]
- [Warning.get_all returns only unique warnings][6372]
- [Reimplement `enso_project` as a proper builtin][6352]
- [Limit number of reported warnings per value][6577]

[3227]: https://github.com/enso-org/enso/pull/3227
[3248]: https://github.com/enso-org/enso/pull/3248
Expand Down Expand Up @@ -857,6 +858,7 @@
[6335]: https://github.com/enso-org/enso/pull/6335
[6372]: https://github.com/enso-org/enso/pull/6372
[6352]: https://github.com/enso-org/enso/pull/6352
[6577]: https://github.com/enso-org/enso/pull/6577

# Enso 2.0.0-alpha.18 (2021-10-12)

Expand Down
5 changes: 5 additions & 0 deletions distribution/lib/Standard/Base/0.0.0-dev/src/Warning.enso
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ type Warning
get_all : Any -> Vector Warning
get_all value = Vector.from_polyglot_array (get_all_array value)

## ADVANCED
Returns `True` if the maximal number of reported warnings for a value has been reached, `False` otherwise.
limit_reached : Any -> Boolean
limit_reached value = @Builtin_Method "Warning.limit_reached"

## PRIVATE
ADVANCED

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,8 @@ from Standard.Base import all
process_to_json_text : Any -> Text
process_to_json_text value =
warnings = Warning.get_all value
text = warnings.map w->w.value.to_display_text
text.to_json
texts = warnings.map w->w.value.to_display_text
vec = case Warning.limit_reached value of
True -> texts + ["Warnings limit reached."]
False -> texts
vec.to_json
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ final class ContextEventsListener(
payload: Api.ExpressionUpdate.Payload.Value.Warnings
): ContextRegistryProtocol.ExpressionUpdate.Payload.Value.Warnings =
ContextRegistryProtocol.ExpressionUpdate.Payload.Value
.Warnings(payload.count, payload.warning)
.Warnings(payload.count, payload.warning, payload.reachedMaxCount)

/** Convert the runtime profiling info to the context registry protocol
* representation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,13 @@ object ContextRegistryProtocol {
*
* @param count the number of attached warnings
* @param value textual representation of the attached warning
* @param reachedMaxCount indicated whether maximal number of warnings has been reached
*/
case class Warnings(count: Int, value: Option[String])
case class Warnings(
count: Int,
value: Option[String],
reachedMaxCount: Boolean
)
}

case class Pending(message: Option[String], progress: Option[Double])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ public class RuntimeOptions {
private static final OptionDescriptor ENABLE_EXECUTION_TIMER_DESCRIPTOR =
OptionDescriptor.newBuilder(ENABLE_EXECUTION_TIMER_KEY, ENABLE_EXECUTION_TIMER).build();

public static final String WARNINGS_LIMIT = optionName("warningsLimit");

@Option(
help = "Maximal number of warnings that can be attached to a value.",
category = OptionCategory.INTERNAL)
public static final OptionKey<Integer> WARNINGS_LIMIT_KEY = new OptionKey<>(100);

private static final OptionDescriptor WARNINGS_LIMIT_DESCRIPTOR =
OptionDescriptor.newBuilder(WARNINGS_LIMIT_KEY, WARNINGS_LIMIT).build();

public static final OptionDescriptors OPTION_DESCRIPTORS =
OptionDescriptors.create(
Arrays.asList(
Expand All @@ -122,7 +132,8 @@ public class RuntimeOptions {
PREINITIALIZE_DESCRIPTOR,
WAIT_FOR_PENDING_SERIALIZATION_JOBS_DESCRIPTOR,
USE_GLOBAL_IR_CACHE_LOCATION_DESCRIPTOR,
ENABLE_EXECUTION_TIMER_DESCRIPTOR));
ENABLE_EXECUTION_TIMER_DESCRIPTOR,
WARNINGS_LIMIT_DESCRIPTOR));

/**
* Canonicalizes the option name by prefixing it with the language name.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,8 +417,13 @@ object Runtime {
*
* @param count the number of attached warnings.
* @param warning textual representation of the attached warning.
* @param reachedMaxCount true when reported a maximal number of allowed warnings, false otherwise.
*/
case class Warnings(count: Int, warning: Option[String])
case class Warnings(
count: Int,
warning: Option[String],
reachedMaxCount: Boolean
)
}

/** TBD
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class ContextFactory {
* location
* @param options additional options for the Context
* @param executionEnvironment optional name of the execution environment to use during execution
* @param warningsLimit maximal number of warnings reported to the user
* @return configured Context instance
*/
def create(
Expand All @@ -42,6 +43,7 @@ class ContextFactory {
useGlobalIrCacheLocation: Boolean = true,
enableAutoParallelism: Boolean = false,
executionEnvironment: Option[String] = None,
warningsLimit: Int = 100,
options: java.util.Map[String, String] = java.util.Collections.emptyMap
): PolyglotContext = {
executionEnvironment.foreach { name =>
Expand All @@ -67,6 +69,10 @@ class ContextFactory {
RuntimeOptions.ENABLE_AUTO_PARALLELISM,
enableAutoParallelism.toString
)
.option(
RuntimeOptions.WARNINGS_LIMIT,
warningsLimit.toString
)
.option("js.foreign-object-prototype", "true")
.out(out)
.in(in)
Expand Down
23 changes: 20 additions & 3 deletions engine/runner/src/main/scala/org/enso/runner/Main.scala
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ object Main {
private val AUTO_PARALLELISM_OPTION = "with-auto-parallelism"
private val SKIP_GRAALVM_UPDATER = "skip-graalvm-updater"
private val EXECUTION_ENVIRONMENT_OPTION = "execution-environment"
private val WARNINGS_LIMIT = "warnings-limit"

private lazy val logger = Logger[Main.type]

Expand Down Expand Up @@ -366,6 +367,16 @@ object Main {
)
.build()

val warningsLimitOption = CliOption.builder
.longOpt(WARNINGS_LIMIT)
.hasArg(true)
.numberOfArgs(1)
.argName("limit")
.desc(
"Specifies a maximal number of reported warnings. Defaults to `100`."
)
.build()

val options = new Options
options
.addOption(help)
Expand Down Expand Up @@ -408,6 +419,7 @@ object Main {
.addOption(autoParallelism)
.addOption(skipGraalVMUpdater)
.addOption(executionEnvironmentOption)
.addOption(warningsLimitOption)

options
}
Expand Down Expand Up @@ -542,7 +554,7 @@ object Main {
* @param enableIrCaches are IR caches enabled
* @param inspect shall inspect option be enabled
* @param dump shall graphs be sent to the IGV
* @apram executionEnvironment optional name of the execution environment to use during execution
* @param executionEnvironment optional name of the execution environment to use during execution
*/
private def run(
path: String,
Expand All @@ -554,7 +566,8 @@ object Main {
enableAutoParallelism: Boolean,
inspect: Boolean,
dump: Boolean,
executionEnvironment: Option[String]
executionEnvironment: Option[String],
warningsLimit: Int
): Unit = {
val file = new File(path)
if (!file.exists) {
Expand Down Expand Up @@ -595,6 +608,7 @@ object Main {
strictErrors = true,
enableAutoParallelism = enableAutoParallelism,
executionEnvironment = executionEnvironment,
warningsLimit = warningsLimit,
options = options
)
if (projectMode) {
Expand Down Expand Up @@ -1111,7 +1125,10 @@ object Main {
line.hasOption(INSPECT_OPTION),
line.hasOption(DUMP_GRAPHS_OPTION),
Option(line.getOptionValue(EXECUTION_ENVIRONMENT_OPTION))
.orElse(Some("live"))
.orElse(Some("live")),
Option(line.getOptionValue(WARNINGS_LIMIT))
.map(Integer.parseInt(_))
.getOrElse(100)
)
}
if (line.hasOption(REPL_OPTION)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4387,7 +4387,9 @@ class RuntimeServerTest
methodPointer =
Some(Api.MethodPointer(moduleName, moduleName, "attach")),
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'"), false)
)
)
),
TestMessages
Expand All @@ -4400,7 +4402,7 @@ class RuntimeServerTest
payload = Api.ExpressionUpdate.Payload.Value(
Some(
Api.ExpressionUpdate.Payload.Value
.Warnings(1, Some("(My_Warning.Value 42)"))
.Warnings(1, Some("(My_Warning.Value 42)"), false)
)
)
),
Expand All @@ -4412,7 +4414,9 @@ class RuntimeServerTest
methodPointer =
Some(Api.MethodPointer(moduleName, moduleName, "attach")),
payload = Api.ExpressionUpdate.Payload
.Value(Some(Api.ExpressionUpdate.Payload.Value.Warnings(2, None)))
.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(2, None, false))
)
),
context.executionComplete(contextId)
)
Expand Down Expand Up @@ -4473,7 +4477,9 @@ class RuntimeServerTest
idX,
ConstantsGen.INTEGER,
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'"), false)
)
)
),
TestMessages
Expand All @@ -4482,7 +4488,9 @@ class RuntimeServerTest
idY,
ConstantsGen.INTEGER,
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'"), false)
)
)
),
TestMessages
Expand All @@ -4491,7 +4499,9 @@ class RuntimeServerTest
idRes,
ConstantsGen.NOTHING,
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'"), false)
)
)
),
context.executionComplete(contextId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2874,7 +2874,9 @@ class RuntimeVisualizationsTest
idMain,
ConstantsGen.INTEGER,
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'y'"), false)
)
)
),
context.executionComplete(contextId)
Expand Down Expand Up @@ -3074,15 +3076,19 @@ class RuntimeVisualizationsTest
)
),
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'x'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'x'"), false)
)
)
),
TestMessages.update(
contextId,
idRes,
s"$moduleName.Newtype",
payload = Api.ExpressionUpdate.Payload.Value(
Some(Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'x'")))
Some(
Api.ExpressionUpdate.Payload.Value.Warnings(1, Some("'x'"), false)
)
),
methodPointer = Some(
Api.MethodPointer(moduleName, s"$moduleName.Newtype", "Mk_Newtype")
Expand Down
Loading

0 comments on commit cf3624c

Please sign in to comment.