diff --git a/CHANGELOG.md b/CHANGELOG.md index 653fe089a5..63f07d81cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Changes in this release: - Refactor protocol - Improved lazy execution for attributes - Add support to run the compiler on windows +- Added exception explainer to compiler for 'modified after freeze' (#876) v 2018.3 (2018-12-07) Changes in this release: diff --git a/MANIFEST.in b/MANIFEST.in index c6da3796e1..2fd0763627 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -3,6 +3,7 @@ graft docs include tox.ini include setup.py +recursive-include src *.j2 recursive-include docs * recursive-include misc * recursive-include tests * diff --git a/requirements.txt b/requirements.txt index e8c5d42443..fdc4090da9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,5 +28,6 @@ tornado==5.1.1 tox==3.7.0 tox-venv==0.3.1 typing==3.6.6 +jinja2==2.10 pep8-naming==0.8.2 -flake8==3.7.5 +flake8==3.7.5 \ No newline at end of file diff --git a/setup.py b/setup.py index b0631893a0..4e7a5976bb 100644 --- a/setup.py +++ b/setup.py @@ -15,6 +15,7 @@ "typing", "PyJWT", "cryptography", + "jinja2", ] setup( diff --git a/src/inmanta/app.py b/src/inmanta/app.py index ce0c687676..ad179d49f2 100755 --- a/src/inmanta/app.py +++ b/src/inmanta/app.py @@ -356,6 +356,10 @@ def cmd_parser(): return parser +def _is_on_tty() -> bool: + return (hasattr(sys.stdout, 'isatty') and sys.stdout.isatty()) or const.ENVIRON_FORCE_TTY in os.environ + + def _get_default_stream_handler(): stream_handler = logging.StreamHandler(stream=sys.stdout) stream_handler.setLevel(logging.INFO) @@ -380,13 +384,13 @@ def _get_watched_file_handler(options): def _convert_to_log_level(level): if level >= len(log_levels): - level = 3 + level = len(log_levels) - 1 return log_levels[level] def _get_log_formatter_for_stream_handler(timed): log_format = "%(asctime)s " if timed else "" - if (hasattr(sys.stdout, 'isatty') and sys.stdout.isatty()) or const.ENVIRON_FORCE_TTY in os.environ: + if _is_on_tty(): log_format += "%(log_color)s%(levelname)-8s%(reset)s %(blue)s%(message)s" formatter = colorlog.ColoredFormatter( log_format, @@ -452,6 +456,12 @@ def report(e): else: sys.excepthook(*sys.exc_info()) + if isinstance(e, CompilerException): + from inmanta.compiler.help.explainer import ExplainerFactory + helpmsg = ExplainerFactory().explain_and_format(e, plain=not _is_on_tty()) + if helpmsg is not None: + print(helpmsg) + try: options.func(options) except CLIException as e: diff --git a/src/inmanta/ast/__init__.py b/src/inmanta/ast/__init__.py index 571775e5a4..80311d79e3 100644 --- a/src/inmanta/ast/__init__.py +++ b/src/inmanta/ast/__init__.py @@ -16,11 +16,10 @@ Contact: code@inmanta.com """ -from inmanta import util - from typing import Dict, Sequence, List, Optional, Union # noqa: F401 from abc import abstractmethod import traceback +from functools import lru_cache try: @@ -31,11 +30,12 @@ if TYPE_CHECKING: import inmanta.ast.statements # noqa: F401 + from inmanta.ast.attribute import Attribute # noqa: F401 from inmanta.ast.type import Type, NamedType # noqa: F401 - from inmanta.execute.runtime import ExecutionContext, Instance # noqa: F401 - from inmanta.ast.statements import Statement # noqa: F401 + from inmanta.execute.runtime import ExecutionContext, Instance, DelayedResultVariable # noqa: F401 + from inmanta.ast.statements import Statement, AssignStatement # noqa: F401 from inmanta.ast.entity import Entity # noqa: F401 - from inmanta.ast.statements.define import DefineImport # noqa: F401 + from inmanta.ast.statements.define import DefineImport, DefineEntity # noqa: F401 class Location(object): @@ -77,7 +77,7 @@ def merge(self, other: Location) -> Location: assert isinstance(other, Location) assert self.file == other.file - if isinstance(other, Location): + if not isinstance(other, Range): return Location(self.file, min(self.lnr, other.lnr)) else: if other.lnr < self.lnr: @@ -135,7 +135,7 @@ class LocatableString(object): 2. in the constructors of other statements """ - def __init__(self, value, location: Range, lexpos: "int", namespace: "Namespace") -> None: + def __init__(self, value: str, location: Range, lexpos: "int", namespace: "Namespace") -> None: self.value = value self.location = location @@ -147,13 +147,13 @@ def __init__(self, value, location: Range, lexpos: "int", namespace: "Namespace" self.lexpos = lexpos self.namespace = namespace - def get_value(self): + def get_value(self) -> str: return self.value - def get_location(self): + def get_location(self) -> Location: return self.location - def __str__(self): + def __str__(self) -> str: return self.value @@ -438,7 +438,7 @@ def _get_ns(self, ns_parts: List[str]) -> "Optional[Namespace]": return None return child._get_ns(ns_parts[1:]) - @util.memoize + @lru_cache() def to_path(self) -> List[str]: """ Return a list with the namespace path elements in it. @@ -456,6 +456,7 @@ def get_location(self) -> Location: class CompilerException(Exception): + """ Base class for exceptions generated by the compiler""" def __init__(self, msg: str) -> None: Exception.__init__(self, msg) @@ -483,7 +484,7 @@ def format(self) -> str: else: return self.get_message() - def format_trace(self, indent="", indent_level=0): + def format_trace(self, indent: str="", indent_level: int=0) -> str: """Make a representation of this exception and its causes""" out = indent * indent_level + self.format() @@ -494,11 +495,12 @@ def format_trace(self, indent="", indent_level=0): return out - def __str__(self): + def __str__(self) -> str: return self.format() class RuntimeException(CompilerException): + """Baseclass for exceptions raised by the compiler after parsing is complete.""" def __init__(self, stmt: "Optional[Locatable]", msg: str) -> None: CompilerException.__init__(self, msg) @@ -507,7 +509,10 @@ def __init__(self, stmt: "Optional[Locatable]", msg: str) -> None: self.set_location(stmt.get_location()) self.stmt = stmt - def set_statement(self, stmt: "Locatable", replace: bool = True): + def set_statement(self, stmt: "Locatable", replace: bool = True) -> None: + for cause in self.get_causes(): + cause.set_statement(stmt, replace) + if replace or self.stmt is None: self.set_location(stmt.get_location()) self.stmt = stmt @@ -520,6 +525,7 @@ def format(self) -> str: class TypeNotFoundException(RuntimeException): + """Exception raised when a type is referenced that does not exist""" def __init__(self, type: str, ns: Namespace) -> None: RuntimeException.__init__(self, stmt=None, msg="could not find type %s in namespace %s" % (type, ns)) @@ -534,6 +540,10 @@ def stringify_exception(exn: Exception) -> str: class ExternalException(RuntimeException): + """ + When a plugin call produces an exception that is not a RuntimeException, + it is wrapped in an ExternalException to make it conform to the expected interface + """ def __init__(self, stmt: Locatable, msg: str, cause: Exception) -> None: RuntimeException.__init__(self, stmt=stmt, msg=msg) @@ -543,7 +553,7 @@ def __init__(self, stmt: Locatable, msg: str, cause: Exception) -> None: def get_causes(self) -> List[CompilerException]: return [] - def format_trace(self, indent="", indent_level=0): + def format_trace(self, indent: str="", indent_level: int=0) -> str: """Make a representation of this exception and its causes""" out = indent * indent_level + self.format() @@ -556,6 +566,7 @@ def format_trace(self, indent="", indent_level=0): class WrappingRuntimeException(RuntimeException): + """ Baseclass for RuntimeExceptions wrapping other RuntimeException """ def __init__(self, stmt: Locatable, msg: str, cause: RuntimeException) -> None: if stmt is None and isinstance(cause, RuntimeException): @@ -570,6 +581,7 @@ def get_causes(self) -> List[CompilerException]: class AttributeException(WrappingRuntimeException): + """ Exception raise when an attribute could not be set, always wraps another exception """ def __init__(self, stmt: "Locatable", instance: "Instance", attribute: str, cause: RuntimeException) -> None: WrappingRuntimeException.__init__( @@ -579,6 +591,7 @@ def __init__(self, stmt: "Locatable", instance: "Instance", attribute: str, caus class OptionalValueException(RuntimeException): + """Exception raised when an optional value is accessed that has no value (and is frozen)""" def __init__(self, instance: "Instance", attribute: str) -> None: RuntimeException.__init__(self, instance, "Optional variable accessed that has no value (%s.%s)" % @@ -588,10 +601,12 @@ def __init__(self, instance: "Instance", attribute: str) -> None: class IndexException(RuntimeException): + """Exception raised when an index definition is invalid""" pass class TypingException(RuntimeException): + """Base class for exceptions raised during the typing phase of compilation""" pass @@ -600,14 +615,16 @@ class KeyException(RuntimeException): class CycleExcpetion(TypingException): + """Exception raised when a type is its own parent (type cycle)""" - def __init__(self, first_type, final_name): + def __init__(self, first_type: "DefineEntity", final_name: str) -> None: super(CycleExcpetion, self).__init__(first_type, None) - self.types = [] + self.types = [] # type: List[DefineEntity] self.complete = False self.final_name = final_name - def add(self, element): + def add(self, element: "DefineEntity") -> None: + """Collect parent entities while traveling up the stack""" if(self.complete): return if element.get_full_name() == self.final_name: @@ -649,7 +666,26 @@ def __init__(self, stmt: "Statement", value: object, location: Location, newvalu RuntimeException.__init__(self, stmt, msg) +class ModifiedAfterFreezeException(RuntimeException): + + def __init__(self, + rv: "DelayedResultVariable", + instance: "Entity", + attribute: "Attribute", + value: object, + location: Location, + reverse: bool) -> None: + RuntimeException.__init__(self, None, "List modified after freeze") + self.instance = instance + self.attribute = attribute + self.value = value + self.location = location + self.resultvariable = rv + self.reverse = reverse + + class DuplicateException(TypingException): + """ Exception raise when something is defined twice """ def __init__(self, stmt: Locatable, other: Locatable, msg: str) -> None: TypingException.__init__(self, stmt, msg) @@ -665,6 +701,7 @@ class CompilerError(Exception): class MultiException(CompilerException): + """A single exception collecting multiple CompilerExceptions""" def __init__(self, others: List[CompilerException]) -> None: CompilerException.__init__(self, "") diff --git a/src/inmanta/ast/statements/generator.py b/src/inmanta/ast/statements/generator.py index 260db79475..7c494e16ea 100644 --- a/src/inmanta/ast/statements/generator.py +++ b/src/inmanta/ast/statements/generator.py @@ -203,6 +203,9 @@ def __init__(self, self._indirect_attributes = {} # type: Dict[str,ExpressionStatement] self._use_default = set() # type: Set[str] + def pretty_print(self) -> str: + return "%s(%s)" % (self.class_type, ",".join(("%s=%s" % (k, v.pretty_print()) for k, v in self.attributes.items()))) + def normalize(self) -> None: mytype = self.namespace.get_type(self.class_type) diff --git a/src/inmanta/compiler/__init__.py b/src/inmanta/compiler/__init__.py new file mode 100644 index 0000000000..22426e2c09 --- /dev/null +++ b/src/inmanta/compiler/__init__.py @@ -0,0 +1,185 @@ +""" + Copyright 2017-2019 Inmanta + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + + Contact: code@inmanta.com +""" + +import os +import sys +import logging +import glob +import imp + +from inmanta.execute import scheduler +from inmanta.ast import Namespace, LocatableString, Range +from inmanta.ast.statements.define import DefineEntity, DefineRelation, PluginStatement +from inmanta.module import Project +from inmanta.plugins import PluginMeta + +LOGGER = logging.getLogger(__name__) + + +def do_compile(refs={}): + """ + Run run run + """ + project = Project.get() + compiler = Compiler(os.path.join(project.project_path, project.main_file), refs=refs) + + LOGGER.debug("Starting compile") + + (statements, blocks) = compiler.compile() + sched = scheduler.Scheduler() + success = sched.run(compiler, statements, blocks) + + LOGGER.debug("Compile done") + + if not success: + sys.stderr.write("Unable to execute all statements.\n") + return (sched.get_types(), compiler.get_ns()) + + +def anchormap(refs={}): + """ + Run run run + """ + project = Project.get() + compiler = Compiler(os.path.join(project.project_path, project.main_file), refs=refs) + + LOGGER.debug("Starting compile") + + (statements, blocks) = compiler.compile() + sched = scheduler.Scheduler() + return sched.anchormap(compiler, statements, blocks) + + +class Compiler(object): + """ + An inmanta compiler + + @param options: Options passed to the application + @param config: The parsed configuration file + """ + + def __init__(self, cf_file="main.cf", refs={}): + self.__init_cf = "_init.cf" + + self.__cf_file = cf_file + self.__root_ns = None + self.refs = refs + + def get_plugins(self): + return self.plugins + + def is_loaded(self): + """ + Is everything loaded and the namespace structure built? + """ + return self.__root_ns is not None + + def get_ns(self): + """ + Get the root namespace + """ + if not self.is_loaded(): + self.load() + return self.__root_ns + + ns = property(get_ns) + + def read(self, path): + """ + Return the content of the given file + """ + with open(path, "r") as file_d: + return file_d.read() + + def _load_plugins(self, plugin_dir, namespace): + """ + Load all modules in plugin_dir + """ + if not os.path.exists(os.path.join(plugin_dir, "__init__.py")): + raise Exception("The plugin directory %s should be a valid python package with a __init__.py file" % plugin_dir) + + mod_name = ".".join(namespace.to_path()) + imp.load_package(mod_name, plugin_dir) + + for py_file in glob.glob(os.path.join(plugin_dir, "*.py")): + if not py_file.endswith("__init__.py"): + # name of the python module + sub_mod = mod_name + "." + os.path.basename(py_file).split(".")[0] + + # create a namespace for the submodule + new_ns = Namespace(sub_mod.split(".")[-1]) + new_ns.parent = namespace + self.graph.add_namespace(new_ns, namespace) + + # load the python file + imp.load_source(sub_mod, py_file) + + def compile(self): + """ + This method will compile and prepare everything to start evaluation + the configuration specification. + + This method will: + - load all namespaces + - compile the __config__ namespace + - start resolving it and importing unknown namespaces + """ + project = Project.get() + self.__root_ns = project.get_root_namespace() + + project.load() + statements, blocks = project.get_complete_ast() + + # load plugins + for name, cls in PluginMeta.get_functions().items(): + + mod_ns = cls.__module__.split(".") + if mod_ns[0] != "inmanta_plugins": + raise Exception("All plugin modules should be loaded in the impera_plugins package not in %s" % cls.__module__) + + mod_ns = mod_ns[1:] + + ns = self.__root_ns + for part in mod_ns: + if ns is None: + break + ns = ns.get_child(part) + + if ns is None: + raise Exception("Unable to find namespace for plugin module %s" % (cls.__module__)) + + cls.namespace = ns + + name = name.split("::")[-1] + statement = PluginStatement(ns, name, cls) + statements.append(statement) + + # add the entity type (hack?) + ns = self.__root_ns.get_child_or_create("std") + nullrange = Range("internal", 1, 0, 0, 0) + entity = DefineEntity(ns, LocatableString("Entity", nullrange, 0, ns), + "The entity all other entities inherit from.", [], []) + str_std_entity = LocatableString("std::Entity", nullrange, 0, ns) + + requires_rel = DefineRelation((str_std_entity, LocatableString("requires", nullrange, 0, ns), [0, None], False), + (str_std_entity, LocatableString("provides", nullrange, 0, ns), [0, None], False)) + requires_rel.namespace = self.__root_ns.get_ns_from_string("std") + + statements.append(entity) + statements.append(requires_rel) + return (statements, blocks) diff --git a/src/inmanta/compiler/help/__init__.py b/src/inmanta/compiler/help/__init__.py new file mode 100644 index 0000000000..daa197f558 --- /dev/null +++ b/src/inmanta/compiler/help/__init__.py @@ -0,0 +1,17 @@ +""" + Copyright 2019 Inmanta + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + + Contact: code@inmanta.com +""" diff --git a/src/inmanta/compiler/help/explainer.py b/src/inmanta/compiler/help/explainer.py new file mode 100644 index 0000000000..86ed13a7af --- /dev/null +++ b/src/inmanta/compiler/help/explainer.py @@ -0,0 +1,167 @@ +""" + Copyright 2018 Inmanta + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + + Contact: code@inmanta.com +""" +from inmanta.ast import CompilerException, ModifiedAfterFreezeException +from inmanta.execute.runtime import OptionVariable +from inmanta.ast.statements import AssignStatement +from inmanta.ast.statements.generator import Constructor + + +from abc import abstractmethod +from typing import Optional, Dict, List, Any +from jinja2 import PackageLoader, Environment +import os +import re + + +def bold(content: str=None) -> str: + if content is None: + return "\033[1m" + return "\033[1m{0}\033[0m".format(content) + + +def underline(content: str=None) -> str: + if content is None: + return "\033[4m" + return "\033[4m{0}\033[0m".format(content) + + +def noformat(content: str=None) -> str: + return "\033[0m" + + +CUSTOM_FILTERS = { + "bold": bold, + "underline": underline, + "noformat": noformat, +} + + +class Explainer(object): + + @abstractmethod + def explain(self, problem: CompilerException) -> List[str]: + pass + + +class JinjaExplainer(Explainer): + + def __init__(self, template: str, acceptable_type): + self.template = template + self.acceptable_type = acceptable_type + + def can_handle(self, problem: CompilerException) -> bool: + return isinstance(problem, self.acceptable_type) + + def get_template(self, problem: CompilerException) -> str: + path = os.path.join(os.path.dirname(__file__), self.template) + with open(path, "r") as fh: + return fh.read() + + def explain(self, problem: CompilerException) -> List[str]: + allcauses = set() + work = [problem] + while(work): + w = work.pop() + allcauses.add(w) + work.extend(w.get_causes()) + + explainable = [c for c in allcauses if self.can_handle(c)] + + if not explainable: + return [] + else: + return [self.do_explain(x) for x in explainable] + + def do_explain(self, problem: CompilerException) -> str: + env = Environment( + loader=PackageLoader('inmanta.compiler.help'), + ) + for name, filter in CUSTOM_FILTERS.items(): + env.filters[name] = filter + + template = env.get_template(self.template) + return template.render(**self.get_arguments(problem)) + + def get_arguments(self, problem: CompilerException) -> Dict[str, Any]: + return {} + + +class ModifiedAfterFreezeExplainer(JinjaExplainer): + + def __init__(self): + super(ModifiedAfterFreezeExplainer, self).__init__("modified_after_freeze.j2", ModifiedAfterFreezeException) + + def build_reverse_hint(self, problem): + if isinstance(problem.stmt, AssignStatement): + return "%s.%s = %s" % (problem.stmt.rhs.pretty_print(), + problem.attribute.get_name(), + problem.stmt.lhs.pretty_print()) + + if isinstance(problem.stmt, Constructor): + # find right parameter: + attr = problem.attribute.end.get_name() + if attr not in problem.stmt.get_attributes(): + attr_rhs = "?" + else: + attr_rhs = problem.stmt.get_attributes()[attr].pretty_print() + return "%s.%s = %s" % (attr_rhs, problem.attribute.get_name(), problem.stmt.pretty_print()) + + def get_arguments(self, problem: CompilerException) -> Dict[str, Any]: + return{ + "relation": problem.attribute.get_name(), + "instance": problem.instance, + "values": problem.resultvariable.value, + "value": problem.value, + "location": problem.location, + "reverse": problem.reverse, + "reverse_example": "" if not problem.reverse else self.build_reverse_hint(problem), + "optional": isinstance(problem.resultvariable, OptionVariable) + } + + +def escape_ansi(line): + ansi_escape = re.compile(r'(\x9B|\x1B\[)[0-?]*[ -/]*[@-~]') + return ansi_escape.sub('', line) + + +class ExplainerFactory(object): + + def get_explainers(self) -> List[Explainer]: + return [ModifiedAfterFreezeExplainer()] + + def explain(self, problem: CompilerException) -> List[str]: + return [explanation for explainer in self.get_explainers() for explanation in explainer.explain(problem)] + + def explain_and_format(self, problem: CompilerException, plain: bool = True) -> Optional[str]: + """ + :param plain: remove tty color codes, only return plain text + """ + raw = self.explain(problem) + if not raw: + return None + else: + pre = """ +\033[1mException explanation +=====================\033[0m +""" + pre += "\n\n".join(raw) + + if not plain: + return pre + else: + return escape_ansi(pre) diff --git a/src/inmanta/compiler/help/templates/modified_after_freeze.j2 b/src/inmanta/compiler/help/templates/modified_after_freeze.j2 new file mode 100644 index 0000000000..2d27a260c4 --- /dev/null +++ b/src/inmanta/compiler/help/templates/modified_after_freeze.j2 @@ -0,0 +1,52 @@ +The compiler could not figure out how to execute this model. + +{% if optional -%} +During compilation, the compiler has to decide when it expects an optional relation to remain undefined. In this compiler run, it guessed that the relation '{{relation | underline}}' on the instance {{ "\033[4m" }}{{instance}}{{ "\033[0m" }} would never get a value assigned, but the value {{value | underline }} was assigned at {{location | underline}} + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + {{ None | bold }}implementation mydefault for MyEntity: + self.relation = "default" + end + + implement MyEntity using mydefault when not (relation is defined){{ None | noformat }} + + This is always wrong, because the relation can not at the same time be undefined and have the value "default". + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following: + +1. Ensure the model is correct by checking that the problematic assignment at {{location | underline}} is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. {%if reverse %}[applies]{%else%}[does not apply here]{%endif%} If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation{%if reverse_example%}: {{reverse_example | underline}}{%endif%}. +4. Simplify the model by relying less on `is defined` but use a boolean instead. +{%else-%} +During compilation, the compiler has to decide when it expects a relation to have all its elements. +In this compiler run, it guessed that the relation '{{relation | underline}}' on the instance {{instance | underline}} would be complete with the values [{{values |join(',') | underline }}], but the value {{value | underline}} was added at {{location | underline}} + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + {{ None | bold }}implementation mydefault for MyEntity: + self.relation += "default" + end + + implement MyEntity using mydefault when std::count(relation) == 0{{ None | noformat }} + + + This is always wrong, because the relation can not at the same time have length 0 and contain the value "default" + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following + +1. Ensure the model is correct by checking that the problematic assignment at {{location | underline }} is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. {%if reverse %}[applies]{%else%}[does not apply here]{%endif%} If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation: {{reverse_example | underline}} +4. Simplify the model by reducing the number of implements calls that pass a list into a plugin function in their when clause. + +{% endif%} diff --git a/src/inmanta/execute/proxy.py b/src/inmanta/execute/proxy.py index 1e8ee3508f..63f418dd86 100644 --- a/src/inmanta/execute/proxy.py +++ b/src/inmanta/execute/proxy.py @@ -25,8 +25,8 @@ class UnsetException(RuntimeException): """ - This exception is thrown when an attribute is read that was not yet - available. + This exception is thrown when an attribute is accessed that was not yet + available (i.e. it has not been frozen yet). """ def __init__(self, msg, instance=None, attribute=None): diff --git a/src/inmanta/execute/runtime.py b/src/inmanta/execute/runtime.py index 4106805a02..ec8a8aed8e 100644 --- a/src/inmanta/execute/runtime.py +++ b/src/inmanta/execute/runtime.py @@ -18,8 +18,9 @@ from inmanta.execute.util import Unknown from inmanta.execute.proxy import UnsetException -from inmanta.ast import RuntimeException, NotFoundException, DoubleSetException, OptionalValueException, AttributeException, \ - Locatable, Location +from inmanta.ast import RuntimeException, NotFoundException, DoubleSetException, OptionalValueException, \ + AttributeException, \ + Locatable, Location, ModifiedAfterFreezeException from inmanta.ast.type import Type from typing import List, Dict, Any, Optional @@ -252,9 +253,23 @@ def set_value(self, value, location, recur=True): return for subvalue in value: if subvalue not in self.value: - raise RuntimeException(None, "List modified after freeze") + raise ModifiedAfterFreezeException( + self, + instance=self.myself, + attribute=self.attribute, + value=value, + location=location, + reverse=not recur + ) else: - raise RuntimeException(None, "List modified after freeze") + raise ModifiedAfterFreezeException( + self, + instance=self.myself, + attribute=self.attribute, + value=value, + location=location, + reverse=not recur + ) if isinstance(value, list): if len(value) == 0: @@ -330,6 +345,15 @@ def __init__(self, attribute, instance, queue: "QueueScheduler"): def set_value(self, value, location, recur=True): assert location is not None if self.hasValue: + if self.value is None: + raise ModifiedAfterFreezeException( + self, + instance=self.myself, + attribute=self.attribute, + value=value, + location=location, + reverse=not recur + ) if self.value != value: raise DoubleSetException(None, self.value, self.location, value, location) diff --git a/src/inmanta/execute/scheduler.py b/src/inmanta/execute/scheduler.py index 6170ffd34f..c682211a35 100644 --- a/src/inmanta/execute/scheduler.py +++ b/src/inmanta/execute/scheduler.py @@ -31,6 +31,7 @@ from inmanta.ast import RuntimeException, MultiException, CycleExcpetion from inmanta.execute.tracking import ModuleTracker import itertools +from typing import Dict, List, Set DEBUG = True LOGGER = logging.getLogger(__name__) @@ -81,15 +82,19 @@ def dump_not_done(self): for i in self.verify_done(): i.dump() - def sort_entities(self, entity_map): - out = [] - loopstack = set() + def sort_entities(self, entity_map: Dict[str, DefineEntity]) -> List[DefineEntity]: + out: List[DefineEntity] = [] + loopstack: Set[str] = set() while len(entity_map) > 0: workon = next(iter(entity_map.keys())) self.do_sort_entities(entity_map, workon, out, loopstack) return out - def do_sort_entities(self, entity_map, name, acc, loopstack): + def do_sort_entities(self, + entity_map: Dict[str, DefineEntity], + name: str, + acc: List[DefineEntity], + loopstack: Set[str]) -> None: nexte = entity_map[name] try: del entity_map[name] diff --git a/src/inmanta/parser/__init__.py b/src/inmanta/parser/__init__.py index 4fdd2c2ca8..9e972e3ac6 100644 --- a/src/inmanta/parser/__init__.py +++ b/src/inmanta/parser/__init__.py @@ -20,6 +20,7 @@ class ParserException(CompilerException): + """Exception occurring during the parsing of the code""" def __init__(self, location: Range, value, msg=None): if msg is None: diff --git a/tests/compiler/test_error_explainer.py b/tests/compiler/test_error_explainer.py new file mode 100644 index 0000000000..6c110fd247 --- /dev/null +++ b/tests/compiler/test_error_explainer.py @@ -0,0 +1,243 @@ +""" + Copyright 2019 Inmanta + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + + Contact: code@inmanta.com +""" +import pytest + +from inmanta import compiler +from inmanta.ast import AttributeException +from inmanta.compiler.help.explainer import ExplainerFactory + + +def test_optional_loop_forward(snippetcompiler): + snippetcompiler.setup_for_snippet( + """ +entity Thing: + string name +end + +implement Thing using std::none + +Thing.other [0:1] -- Thing + +implementation setother for Thing: + self.other = Thing(name="it") +end + +implement Thing using setother when not (other is defined) + +Thing(name="a") +""" + ) + with pytest.raises(AttributeException) as e: + compiler.do_compile() + + assert ExplainerFactory().explain_and_format(e.value) == """ +Exception explanation +===================== +The compiler could not figure out how to execute this model. + +During compilation, the compiler has to decide when it expects an optional relation to remain undefined. In this compiler run, it guessed that the relation 'other' on the instance __config__::Thing (instantiated at %(dir)s/main.cf:16) would never get a value assigned, but the value __config__::Thing (instantiated at %(dir)s/main.cf:11) was assigned at %(dir)s/main.cf:11 + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + implementation mydefault for MyEntity: + self.relation = "default" + end + + implement MyEntity using mydefault when not (relation is defined) + + This is always wrong, because the relation can not at the same time be undefined and have the value "default". + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following: + +1. Ensure the model is correct by checking that the problematic assignment at %(dir)s/main.cf:11 is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. [does not apply here] If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation. +4. Simplify the model by relying less on `is defined` but use a boolean instead. +""" % {"dir": snippetcompiler.project_dir} # noqa: E501 + + +def test_optional_loop_forward_tty(snippetcompiler): + snippetcompiler.setup_for_snippet( + """ +entity Thing: + string name +end + +implement Thing using std::none + +Thing.other [0:1] -- Thing + +implementation setother for Thing: + self.other = Thing(name="it") +end + +implement Thing using setother when not (other is defined) + +Thing(name="a") +""" + ) + with pytest.raises(AttributeException) as e: + compiler.do_compile() + + value = ExplainerFactory().explain_and_format(e.value, plain=False) + + assert value == """ +\033[1mException explanation +=====================\033[0m +The compiler could not figure out how to execute this model. + +During compilation, the compiler has to decide when it expects an optional relation to remain undefined. In this compiler run, it guessed that the relation '\033[4mother\033[0m' on the instance \033[4m__config__::Thing (instantiated at %(dir)s/main.cf:16)\033[0m would never get a value assigned, but the value \033[4m__config__::Thing (instantiated at %(dir)s/main.cf:11)\033[0m was assigned at \033[4m%(dir)s/main.cf:11\033[0m + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + \033[1mimplementation mydefault for MyEntity: + self.relation = "default" + end + + implement MyEntity using mydefault when not (relation is defined)\033[0m + + This is always wrong, because the relation can not at the same time be undefined and have the value "default". + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following: + +1. Ensure the model is correct by checking that the problematic assignment at \033[4m%(dir)s/main.cf:11\033[0m is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. [does not apply here] If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation. +4. Simplify the model by relying less on `is defined` but use a boolean instead. +""" % {"dir": snippetcompiler.project_dir} # noqa: E501 + + +def test_optional_loop_reverse(snippetcompiler): + snippetcompiler.setup_for_snippet( + """ +entity Thing: + string name +end + +implement Thing using std::none + +Thing.other [0:1] -- Thing.that [0:] + +implementation setother for Thing: + t = Thing(name="it") + t.that = self +end + +implement Thing using setother when not (other is defined) + +Thing(name="a") +""" + ) + with pytest.raises(AttributeException) as e: + compiler.do_compile() + + assert ExplainerFactory().explain_and_format(e.value) == """ +Exception explanation +===================== +The compiler could not figure out how to execute this model. + +During compilation, the compiler has to decide when it expects an optional relation to remain undefined. In this compiler run, it guessed that the relation 'other' on the instance __config__::Thing (instantiated at %(dir)s/main.cf:17) would never get a value assigned, but the value __config__::Thing (instantiated at %(dir)s/main.cf:11) was assigned at %(dir)s/main.cf:12:14 + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + implementation mydefault for MyEntity: + self.relation = "default" + end + + implement MyEntity using mydefault when not (relation is defined) + + This is always wrong, because the relation can not at the same time be undefined and have the value "default". + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following: + +1. Ensure the model is correct by checking that the problematic assignment at %(dir)s/main.cf:12:14 is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. [applies] If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation: self.other = t. +4. Simplify the model by relying less on `is defined` but use a boolean instead. +""" % {"dir": snippetcompiler.project_dir} # noqa: E501 + + +def test_optional_loop_list(snippetcompiler): + snippetcompiler.setup_for_snippet( + """ +entity Thing: + string name +end + +implement Thing using std::none + +Thing.other [0:] -- Thing.that [0:] + +implementation setother for Thing: + t = Thing(name="it") + t.that = self +end + +implement Thing using setother when std::count(other) == 1 + +t = Thing(name="a") +t.other = Thing(name="b") +""" + ) + with pytest.raises(AttributeException) as e: + compiler.do_compile() + + print(ExplainerFactory().explain_and_format(e.value)) + assert ExplainerFactory().explain_and_format(e.value) == """ +Exception explanation +===================== +The compiler could not figure out how to execute this model. + +During compilation, the compiler has to decide when it expects a relation to have all its elements. +In this compiler run, it guessed that the relation 'other' on the instance __config__::Thing (instantiated at %(dir)s/main.cf:17) would be complete with the values [__config__::Thing (instantiated at %(dir)s/main.cf:18)], but the value __config__::Thing (instantiated at %(dir)s/main.cf:11) was added at %(dir)s/main.cf:12:14 + +This can mean one of two things: + +1. The model is incorrect. Most often, this is due to something of the form: + + implementation mydefault for MyEntity: + self.relation += "default" + end + + implement MyEntity using mydefault when std::count(relation) == 0 + + + This is always wrong, because the relation can not at the same time have length 0 and contain the value "default" + +2. The model is too complicated for the compiler to resolve. + +The procedure to solve this is the following + +1. Ensure the model is correct by checking that the problematic assignment at %(dir)s/main.cf:12:14 is not conditional on the value it assigns. +2. Report a bug to the inmanta issue tracker at https://github.com/inmanta/inmanta/issues or directly contact inmanta. This is a priority issue to us, so you will be helped rapidly and by reporting the problem, we can fix it properly. +3. [applies] If the exception is on the reverse relation, try to give a hint by explicitly using the problematic relation: self.other = t +4. Simplify the model by reducing the number of implements calls that pass a list into a plugin function in their when clause. + +""" % {"dir": snippetcompiler.project_dir} # noqa: E501 diff --git a/tests/compiler/test_null.py b/tests/compiler/test_null.py index bcd08adad6..0d55650198 100644 --- a/tests/compiler/test_null.py +++ b/tests/compiler/test_null.py @@ -133,7 +133,7 @@ def test_null_err(snippetcompiler): """, """Could not set attribute `a` on instance `__config__::A (instantiated at {dir}/main.cf:6)` (reported in Construct(A) ({dir}/main.cf:6)) caused by: - Invalid value 'null', expected String""", # noqa: E501 + Invalid value 'null', expected String (reported in Construct(A) ({dir}/main.cf:6))""", # noqa: E501 ) @@ -148,5 +148,5 @@ def test_null_on_list_err(snippetcompiler): """, """Could not set attribute `a` on instance `__config__::A (instantiated at {dir}/main.cf:6)` (reported in Construct(A) ({dir}/main.cf:6)) caused by: - Invalid value 'null', expected list""", # noqa: E501 + Invalid value 'null', expected list (reported in Construct(A) ({dir}/main.cf:6))""", # noqa: E501 ) diff --git a/tests/test_compilation.py b/tests/test_compilation.py index 8f66f864f3..91b0b63ce8 100644 --- a/tests/test_compilation.py +++ b/tests/test_compilation.py @@ -183,5 +183,5 @@ def test_compile(self): print(text) assert text == """Exception in plugin test::badtype (reported in test::badtype(c1.items) ({dir}/invalid.cf:16)) caused by: - Invalid type for value 'a', should be type test::Item""".format( + Invalid type for value 'a', should be type test::Item (reported in test::badtype(c1.items) ({dir}/invalid.cf:16))""".format( dir=self.project_dir)