Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't throw an exception when a field is missing #17203

Merged
merged 1 commit into from Dec 7, 2019

Conversation

@abcdefg30
Copy link
Member

abcdefg30 commented Oct 8, 2019

Closes #17202.

Testcase: Add JunkTrait: to the world actor in ./mods/ra/maps/allies-01/rules.yaml.
On bleed:

> OpenRA.Utility.exe ra --check-yaml ./mods/ra/maps/allies-01
OpenRA.Utility(1,1): Error: Missing Type: JunkTraitInfo
Testing map: 01:   In the thick of it
OpenRA.Utility(1,1): Error: System.AggregateException: Mindestens ein Fehler ist aufgetreten. ---> System.NullReferenceException: Der Objektverweis wurde nicht auf eine Objektinstanz festgelegt.
   bei System.Object.GetType()
   bei OpenRA.FieldLoader.Load(Object self, MiniYaml my)
   bei OpenRA.ActorInfo.LoadTraitInfo(ObjectCreator creator, String traitName, MiniYaml my)
   bei OpenRA.ActorInfo..ctor(ObjectCreator creator, String name, MiniYaml node)
   bei OpenRA.Ruleset.<>c__DisplayClass14_0.<Load>b__1(MiniYamlNode k)
   bei OpenRA.Exts.ToDictionaryWithConflictLog[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, String debugName, Func`2 logKey, Func`2 logValue)
   bei OpenRA.Ruleset.MergeOrDefault[T](String name, IReadOnlyFileSystem fileSystem, IEnumerable`1 files, MiniYaml additional, IReadOnlyDictionary`2 defaults, Func`2 makeObject, Func`2 filterNode)
   bei OpenRA.Ruleset.<>c__DisplayClass14_0.<Load>b__0()
   bei System.Threading.Tasks.Task.Execute()
   --- Ende der internen Ausnahmestapelüberwachung ---
   bei System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   bei System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   bei OpenRA.Ruleset.Load(ModData modData, IReadOnlyFileSystem fileSystem, String tileSet, MiniYaml mapRules, MiniYaml mapWeapons, MiniYaml mapVoices, MiniYaml mapNotifications, MiniYaml mapMusic, MiniYaml mapSequences, MiniYaml mapModelSequences)
   bei OpenRA.Map.PostInit()
---> (Interne Ausnahme #0) System.NullReferenceException: Der Objektverweis wurde nicht auf eine Objektinstanz festgelegt.
   bei System.Object.GetType()
   bei OpenRA.FieldLoader.Load(Object self, MiniYaml my)
   bei OpenRA.ActorInfo.LoadTraitInfo(ObjectCreator creator, String traitName, MiniYaml my)
   bei OpenRA.ActorInfo..ctor(ObjectCreator creator, String name, MiniYaml node)
   bei OpenRA.Ruleset.<>c__DisplayClass14_0.<Load>b__1(MiniYamlNode k)
   bei OpenRA.Exts.ToDictionaryWithConflictLog[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, String debugName, Func`2 logKey, Func`2 logValue)
   bei OpenRA.Ruleset.MergeOrDefault[T](String name, IReadOnlyFileSystem fileSystem, IEnumerable`1 files, MiniYaml additional, IReadOnlyDictionary`2 defaults, Func`2 makeObject, Func`2 filterNode)
   bei OpenRA.Ruleset.<>c__DisplayClass14_0.<Load>b__0()
   bei System.Threading.Tasks.Task.Execute()<---

This PR:

> OpenRA.Utility.exe ra --check-yaml ./mods/ra/maps/allies-01
OpenRA.Utility(1,1): Error: Missing Type: JunkTraitInfo
Testing map: 01:   In the thick of it
Errors: 1
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Oct 8, 2019

This shouldn't be included in the game, only within Utility at best.

If a trait ends up missing, that can easily be hidden within the game, and this also goes against the mindset which ended up enforcing typoed junk trait tags being logged.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 8, 2019

This shouldn't be included in the game, only within Utility at best.

This is exactly what is happening here. Maybe the title is a bit misleading but if you read the actual code you'll see that the utility overwrites MissingTypeAction not to throw but only log an error instead. This PR is fixing a crash that happens as result of only logging instead of crashing. The normal game will still crash for junk values.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 15, 2019

As discussed in IRC, I have some reservations about the way this is implemented and think we should spend some more time looking at the problem before merging this.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Nov 15, 2019

I am not sure what our alternative here is. As I told you already on IRC, we can't simply try-catch the error during the lint run (or crash without catching), since that would abort the rule parsing. (This is exactly what @chrisf wanted to prevent when he introduced this change way back in 5d1ee14!)

However, since both you and @GraionDilach completely misunderstood the code and changes here after giving them a quick look, I'd suggest making what is going on in ObjectCreator a bit more obvious:
Default MissingTypeAction to null and change

MissingTypeAction(className);

to something like

// HACK: The linter does not want to crash but only print an error instead
if (MissingTypeAction != null)
	MissingTypeAction(className);
else
	throw new InvalidOperationException("Cannot locate type: {0}".F(className));

That way it becomes more obvious that only the linter is omitting the exception. The added null checks will have to stay then though.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 15, 2019

Sounds good to me. Can we please also have comment(s?) in ActorInfo clarifying that CreateObject will throw by default, or return null if MissingTypeAction is set from the linter?

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Nov 21, 2019

Updated with comments.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 29, 2019

There have been updates sine @Mailaender approved, so I think this still needs another approval.

@teinarss teinarss merged commit 9a5eaa7 into OpenRA:bleed Dec 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30 abcdefg30 deleted the abcdefg30:missingNull branch Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.